Skip to content
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

Add function and map method for drawing extent of another wcs #7851

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

wtbarnes
Copy link
Member

@wtbarnes wtbarnes commented Oct 26, 2024

Fixes #7615

TODOs:

  • Figure tests
  • Docs for map method
  • Better docs for visualization function
  • Changelog

@wtbarnes wtbarnes marked this pull request as ready for review October 28, 2024 02:38
@wtbarnes wtbarnes requested review from a team as code owners October 28, 2024 02:38
@wtbarnes wtbarnes added this to the 6.1.0 milestone Oct 28, 2024
@wtbarnes
Copy link
Member Author

Docs build and figure test devdeps fails are unrelated.

@pytest.fixture
def cropped_aia193_sample_map():
import sunpy.data.sample
m = sunpy.map.Map(sunpy.data.sample.AIA_193_JUN2012)
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this sample data file?!

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

@ayshih ayshih left a comment

Choose a reason for hiding this comment

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

  • I think the map method should be locked to plotting that map's own extent on the specified axes. I would not accept a potentially unrelated WCS as an argument.
  • Instead of using the resolution approach, I think it would be better to literally use the pixel corners. That way, the segmentation of the extent box will not be mismatched from pixelization. (There can be a different parameter for adding sampling points along a pixel edge, but the default would be just the corners.)

@wtbarnes
Copy link
Member Author

wtbarnes commented Oct 29, 2024

  • I think the map method should be locked to plotting that map's own extent on the specified axes. I would not accept a potentially unrelated WCS as an argument.

Agreed. This is also more consistent with the way the other draw methods work, i.e. they "draw themselves". I don't know why I did it backwards. Are you opposed to keeping the function in sunpy.visualizations.drawing which takes in any WCS and tries to its extent on an axis?

  • Instead of using the resolution approach, I think it would be better to literally use the pixel corners. That way, the segmentation of the extent box will not be mismatched from pixelization. (There can be a different parameter for adding sampling points along a pixel edge, but the default would be just the corners.)

I see what you mean re: pixelization. However, aren't the four corners alone insufficient to adequately define the map extent? For example, in your original issue, the red and green boxes you show have the same corner coordinates even though the FOVs are different. Am I missing something?

@ayshih
Copy link
Member

ayshih commented Oct 29, 2024

  • I think the map method should be locked to plotting that map's own extent on the specified axes. I would not accept a potentially unrelated WCS as an argument.

Agreed. This is also more consistent with the way the other draw methods work, i.e. they "draw themselves". I don't know why I did it backwards. Are you opposed to keeping the function in sunpy.visualizations.drawing which takes in any WCS and tries to its extent on an axis?

Nope, not opposed as it is consistent with the limb stuff.

  • Instead of using the resolution approach, I think it would be better to literally use the pixel corners. That way, the segmentation of the extent box will not be mismatched from pixelization. (There can be a different parameter for adding sampling points along a pixel edge, but the default would be just the corners.)

I see what you mean re: pixelization. However, aren't the four corners alone insufficient to adequately define the map extent? For example, in your original issue, the red and green boxes you show have the same corner coordinates even though the FOVs are different. Am I missing something?

To clarify, I meant the outer corners of every edge pixel, so 2N+2M(+1) points for an NxM array. For that example image, I think I may have called sunpy.map.maputils.map_edges() (although be careful about the being off by half a pixel).

@wtbarnes
Copy link
Member Author

I've removed the resolution argument and constructed the edges such that the spacing along any given edge has a resolution of 1 pixel, with the left endpoint included and the right endpoint excluded as the latter is the starting point of the subsequent edge.

The reason I didn't use sunpy.map.map_edges was to avoid tying sunpy.visualizations.drawing.extent to GenericMap and instead just accept a WCS. I suppose we could refactor map_edges to use some more general function for finding the extent of the WCS and then use that in both functions.

@@ -194,6 +194,54 @@ def prime_meridian(axes, *, rsun: u.m = R_sun, resolution=500, **kwargs):
return visible, hidden


@u.quantity_input
def extent(axes, wcs, *, rsun: u.m = R_sun, **kwargs):
Copy link
Member

Choose a reason for hiding this comment

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

Without having looked at this in detail, this feels like something which could be useful to upstream to astropy?

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably. I'm kind of surprised this doesn't already exist actually. Hopefully I'm not duplicating functionality here.

@Cadair Cadair self-requested a review October 30, 2024 07:34
@wtbarnes wtbarnes requested a review from ayshih November 6, 2024 16:38
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.

Add a Map method to draw the extent of the map
4 participants