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

Force map.meta to be a MetaDict #4476

Merged
merged 6 commits into from
Sep 23, 2020
Merged

Conversation

dstansby
Copy link
Member

@dstansby dstansby commented Sep 14, 2020

This is a requirement for #4467.

Previously it was possible that map.meta could just be a simple dict if a map was constructed with map.GenericMap. This brings GenericMap in line with the map factory (ie. Map) and forces meta to always be a MetaDict.

@dstansby dstansby requested a review from a team as a code owner September 14, 2020 15:13
@nabobalis nabobalis added this to the 2.1 milestone Sep 14, 2020
@nabobalis nabobalis added map Affects the map submodule util Issues relating to sunpy.util labels Sep 14, 2020
Comment on lines -904 to -910
def test_bad_header_final_fallback():
# Checks that if a WCS cannot be constructed from the
# header, a warning is raised and a simple WCS is created
# instead
Copy link
Member Author

Choose a reason for hiding this comment

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

As far as I can tell, as long as everything in the header is a string (which is now mandated), astropy will happily parse it and we will never get to the point where a simple WCS needs to be created. Warnings like

VerifyWarning("Keyword name '#none' is greater than 8 characters or contains characters not allowed by the FITS standard; a HIERARCH card will be created.")

are raised when I try and put some gibberish in a header. So I've removed this test. I suppose there's no harm in have the fallback, but I can't think of anything that will actually exercise it.

Copy link
Member

Choose a reason for hiding this comment

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

Interesting, and it's good that this improves the validation and makes the error better.

I think there is a class of errors for which the fallback in GenericMap is still useful and might not be being tested, which is a valid FITS header but an invalid WCS. i.e. something which when passed to astropy.wcs.WCS errors because of some issue (like invalid values etc).

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm still struggling to find something that trips the warning. Changing ctype, crval, crpix, or cdelt to garbage values just makes the checks below error instead. I've also tried with the header (I think a WISPR one?) that caused this warning to be put in the code, and that doesn't trigger the warning either.

@nabobalis nabobalis added the Needs Review Needs reviews before merge. label Sep 21, 2020
@nabobalis
Copy link
Contributor

nabobalis commented Sep 22, 2020

I feel like this is a very slight API change, backporting this might be off the cards.

@Cadair Cadair merged commit d2df489 into sunpy:master Sep 23, 2020
@dstansby dstansby deleted the force-metadict branch September 23, 2020 17:01
@dstansby dstansby removed the Needs Review Needs reviews before merge. label Jan 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
map Affects the map submodule util Issues relating to sunpy.util
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants