New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduced area-based contours #7444
base: main
Are you sure you want to change the base?
Conversation
Thank you for the PR @Paras20222. I will do a quick review now. |
sunpy/map/mapbase.py
Outdated
|
||
def _calculate_contour_levels_by_area(self, levels): | ||
""" | ||
Calculate contour levels based on area containment. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure the word containment is correct here.
sunpy/map/mapbase.py
Outdated
>>> map_data = np.random.rand(100, 100) | ||
>>> quantiles = [0.1, 0.3, 0.5, 0.7, 0.9] | ||
>>> contour_levels = self._calculate_contour_levels_by_area(quantiles) | ||
>>> print(contour_levels) | ||
[0.3160296 0.54729944 0.70897832 0.83778233 0.95295992] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the imports are missing?
Also the print is not needed here. The doctest will print out the return if you do not assign it to a variable.
@@ -2305,7 +2348,9 @@ def _process_levels_arg(self, levels): | |||
"it should be an Astropy Quantity object.") | |||
|
|||
if levels.unit == u.percent: | |||
return 0.01 * levels.to_value('percent') * np.nanmax(self.data) | |||
# Calculate contour levels based on area containment | |||
contour_levels = self._calculate_contour_levels_by_area(levels) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't change the default behavior of this method.
I think for now, adding a "method" keyword (to the draw contours method) that allows you to select this new method would be better.
@@ -2290,6 +2290,49 @@ def draw_quadrangle(self, bottom_left, *, width: (u.deg, u.pix) = None, height: | |||
quad = Quadrangle(anchor, width, height, **kwergs) | |||
axes.add_patch(quad) | |||
return quad | |||
|
|||
def _calculate_contour_levels_by_area(self, levels): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will also need to be tested both as a normal unit test but also as a figure test. See https://docs.sunpy.org/en/latest/dev_guide/contents/tests.html for more details.
Co-authored-by: Nabil Freij <nabil.freij@gmail.com>
Co-authored-by: Nabil Freij <nabil.freij@gmail.com>
Hi @Paras20222, will you have some time soon to address the outstanding comments? |
Hello, i have made few changes to the gist. def area_contour_image(img: np.ndarray, quantiles: list[float], ax=None, x=None, y=None, cmap='Reds', **contour_kwds):
def test():
if name == 'main': |
Modifying the gist is not the most useful, we can't make suggestions nor does it move this pull request forward. I would suggest that you take over this pull request, you will need to fetch the original comments from this pull request onto your sunpy fork, then submit a new pull request with any changes you think need to be made as well addressing the comments that i left here? Does that sound ok? We can also help with any of the steps if you get stuck. |
Sure, I would be happy to work upon it. I am new to open source contributions, so I am thankful for your words of guidance. |
PR Description
This PR introduces area-based contour calculations to enhance visualization accuracy. The changes address issue #7420 and provide a more intuitive method for determining contour levels. This improvement enables better data interpretation in X-ray and radio imaging applications.