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

Prioritize hgln_obs over crln_obs when extracting observer information from FITS headers #7188

Merged
merged 4 commits into from Sep 15, 2023

Conversation

svank
Copy link
Contributor

@svank svank commented Sep 8, 2023

Fix as proposed in #7184

Thanks @ayshih for confirming this should be right!

@svank svank requested a review from a team as a code owner September 8, 2023 19:00
Copy link
Member

@dstansby dstansby left a comment

Choose a reason for hiding this comment

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

Looks 👍 , just one request for a comment, and needs a bugifx changelog entry adding.

sunpy/coordinates/wcs_utils.py Show resolved Hide resolved
@svank
Copy link
Contributor Author

svank commented Sep 11, 2023

@dstansby Thanks! I've made these changes.

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.

Some text edits, otherwise looks good to me!

If I were to be super-picky, the part of the unit test that tests the prioritization in the Map implementation should really live under sunpy.map.tests and not sunpy.coordinates.tests, because the relevant code – how GenericMap prioritizes and sanitizes the observer location – all lives in sunpy/map/mapbase.py.

@@ -0,0 +1 @@
Ensure that when instantiating a `WCS` from a FITS Header, the observer location is set from the Stonyhurst coordinates rather than the Carrington coordinates when both are present.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Ensure that when instantiating a `WCS` from a FITS Header, the observer location is set from the Stonyhurst coordinates rather than the Carrington coordinates when both are present.
When directly instantiating a `~astropy.wcs.WCS` from a FITS header that contains both Stonyhurst and Carrington heliographic coordinates for the observer location, the Stonyhurst coordinates will now be prioritized.
This behavior is now consistent with the `~sunpy.map.Map` class, which has always prioritized Stonyhurst coordinates over Carrington coordinates.

Comment on lines 408 to 409
Going through `Map` and `WCS` seem to follow (at least slightly) different
code paths, so test them both.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Going through `Map` and `WCS` seem to follow (at least slightly) different
code paths, so test them both.
`Map` creates a custom FITS header with a sanitized observer location, so
we need to also test a directly instantiated `WCS` object.

@ayshih ayshih added coordinates Affects the coordinates submodule backport 5.0 on-merge: backport to 5.0 labels Sep 11, 2023
@Cadair
Copy link
Member

Cadair commented Sep 12, 2023

the part of the unit test that tests the prioritization in the Map implementation should really live under sunpy.map.tests

Given my recent foray into seeing if it's possible to run tests for subpackages without optional dependencies installed, ideally we would not have a direct reliance on map in the coordinates tests.

@dstansby dstansby dismissed their stale review September 12, 2023 14:49

Changes addressed

@svank svank requested a review from a team as a code owner September 12, 2023 21:32
@svank
Copy link
Contributor Author

svank commented Sep 12, 2023

What's code review for, if not being picky? I moved the Map part of the test to the map module---there is still a pre-existing Map dependence in coordinates/tests/test_wcs_utils.py, but now I avoid further entrenching that.

Text edits are also implemented.

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.

Looks good to me!

@ayshih ayshih added the Minor Change PR only needs one approval to merge label Sep 15, 2023
@ayshih ayshih merged commit 7f1ae7b into sunpy:main Sep 15, 2023
23 of 26 checks passed
meeseeksmachine pushed a commit to meeseeksmachine/sunpy that referenced this pull request Sep 15, 2023
@ayshih
Copy link
Member

ayshih commented Sep 15, 2023

Thanks, @svank!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 5.0 on-merge: backport to 5.0 coordinates Affects the coordinates submodule Minor Change PR only needs one approval to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants