Skip to content

Conversation

@FionnStack
Copy link
Collaborator

Updates to ChimeraPy

@codecov
Copy link

codecov bot commented Apr 9, 2025

Codecov Report

❌ Patch coverage is 63.80952% with 38 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.14%. Comparing base (78e4102) to head (b7f5b38).
⚠️ Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
chimerapy/chimera.py 58.53% 34 Missing ⚠️
chimerapy/chimera_original.py 69.23% 4 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (78e4102) and HEAD (b7f5b38). Click for more details.

HEAD has 4 uploads less than BASE
Flag BASE (78e4102) HEAD (b7f5b38)
5 1
Additional details and impacted files
@@             Coverage Diff             @@
##             main      #49       +/-   ##
===========================================
- Coverage   98.55%   88.14%   -10.42%     
===========================================
  Files           2        3        +1     
  Lines         277      371       +94     
===========================================
+ Hits          273      327       +54     
- Misses          4       44       +40     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member

@samaloney samaloney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work definitely on the right track!

final_mask = mask_171_211 * mask_211_193 * mask_171_193
return final_mask

def filter_by_area(mask, min_area=100):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the area here is in pixels I assume it would be better and more flexible to specify as an area also good to use quantity decorator.

A list of dictionaries containing properties of each coronal hole.
"""
coronal_holes = []
labeled_mask = measure.label(ch_mask)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So you are calling measure.label twice one in filter_by_area and again here could you return the filtered regions and mask from filter_by_area and call it in this function or before?

Comment on lines 109 to 113
min_row, min_col, max_row, max_col = region.bbox
width_pixels = {
"E-W": max_col - min_col,
"N-S": max_row - min_row
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about transforming all the pixels of the contour to world coordinates first, then transform to heliographic?

I also think want to keep a few of the intermediate values you calculate below e.g min/max of lat and lon as well as the difference or extent

Comment on lines 116 to 117
bottom_left = map_obj.pixel_to_world(min_col * u.pix, min_row * u.pix).transform_to("heliographic_stonyhurst")
top_right = map_obj.pixel_to_world(max_col * u.pix, max_row * u.pix).transform_to("heliographic_stonyhurst")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I have a pixel off the solar disk what would the corresponding lat and lon be in Heliogprohic?


centroid = region.centroid
centroid_world = map_obj.pixel_to_world(centroid[1] * u.pix, centroid[0] * u.pix)
area_percentage = (region.area / solar_disk_area_pixels) * 100
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be an area e.g m**2 can convert to other units later

Comment on lines 103 to 106
solar_radius_arcsec = map_obj.rsun_obs
pixel_scale_arcsec = map_obj.scale[0]
solar_radius_pixels = (solar_radius_arcsec / pixel_scale_arcsec).value
solar_disk_area_pixels = np.pi * (solar_radius_pixels ** 2)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do this account for the difference between pixel near the middle of the disk compare to the edge - remember we're projecting as sphere, the sun, onto a plane?

Copy link
Member

@samaloney samaloney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nearly there just some small issues on the area calculations.

im_map : `~sunpy.map.Map`
Processed magnetogram map.
"""
im_map.data[~coordinate_is_on_solar_disk(all_coordinates_from_map(im_map))] = np.nan
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is actually changing the data array is that what you want?

Not important now but coordinate_is_on_solar_disk(all_coordinates_from_map(im_map) is an expensive operation maybe could calculate once and store?

Comment on lines 191 to 193
m171_corrected = m171.data * cos_correction_171
m193_corrected = m193.data * cos_correction_193
m211_corrected = m211.data * cos_correction_211
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What data is being corrected here?

cos_correction = np.ones_like(im_map.data)

radial_angle = np.arccos(np.cos(coordinates.Tx[on_disk]) * np.cos(coordinates.Ty[on_disk]))
cos_cor_ratio = (radial_angle / im_map.rsun_obs).value
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are units of radial_angle and im_map.rsun_obs, do the values of cos_cor_ratio make sense?


cos_correction = np.ones_like(im_map.data)

radial_angle = np.arccos(np.cos(coordinates.Tx[on_disk]) * np.cos(coordinates.Ty[on_disk]))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you want the output at the end to be a list or a 2d array?

@u.quantity_input(min_area=u.m**2)
def filter_by_area(mask, map_obj, solar_radius_pixels, min_area=1e10 * u.m**2):
solar_radius_meters = map_obj.rsun_meters
pixel_scale_arcsec = map_obj.scale[0].value
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Careful with value if you really need a raw number use to_value, but is that what you really want here?

pixel_scale_arcsec = map_obj.scale[0].value
pixel_scale_meters = (pixel_scale_arcsec * u.arcsec).to(u.rad).value * solar_radius_meters
pixel_area_meters2 = pixel_scale_meters**2
min_area_pixels = (min_area / pixel_area_meters2).decompose().value
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this account for the cosine correction?

m193 = map_threshold(m193)
m211 = map_threshold(m211)

cos_correction_171 = calculate_cosine_correction(m171)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are the values in the maps that go into the cosine correction very different, could be calculate only once?

Comment on lines +120 to +121
coords = region.coords
world_coords = map_obj.pixel_to_world(coords[:, 1] * u.pix, coords[:, 0] * u.pix)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe?

Suggested change
coords = region.coords
world_coords = map_obj.pixel_to_world(coords[:, 1] * u.pix, coords[:, 0] * u.pix)
coords = region.coords * u.pix
world_coords = map_obj.pixel_to_world(coords[:, 1], coords[:, 0])

@FionnStack
Copy link
Collaborator Author

auto fix

@samaloney
Copy link
Member

pre-commit.ci autofix

@samaloney samaloney merged commit d6a5f8b into TCDSolar:main Jun 4, 2025
8 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants