Allow DN as a unit on GenericMap
#7585
Conversation
nabobalis
added
map
|
I approve of the principle of this PR. I think that this is a case of the FITS standard being insufficient, rather than solar physicists not adhering to that standard. We need DN. It is not the same as counts. |
|
The only tests I could not fix were the asdf ones. |
|
The figure test fails are real and are due to the unit label changing to "DN" |
| unit = None | ||
| # NOTE: Special case DN here as it is not part of the FITS standard, but | ||
| # is widely used and is also a recognized astropy unit | ||
| unit_str_components = unit.to_string().replace('/', ' ').replace('(', '').replace(')', '') |
There was a problem hiding this comment.
What on earth caused you to need this line? This is great and I am here for it.
There was a problem hiding this comment.
I guess I removed the comment explaining this. Because anything with DN is not FITS-compliant, the whole unit becomes an UnrecognizedUnit and you cannot decompose it into individual Unit objects. Thus, you have to parse it as a string. The replace methods remove non unit characters that could be present in the string.
The goal here is to breakup the unit into components to check if one of them is DN. Just checking for "DN" in the unit string is ambiguous because it's possible some unit could include a contiguous "dn" in it somewhere.
If there is a better way to do this, I'm all for it. I don't like this approach.
There was a problem hiding this comment.
I guess I removed the comment explaining this. Because anything with DN is not FITS-compliant, the whole unit becomes an
UnrecognizedUnitand you cannot decompose it into individualUnitobjects. Thus, you have to parse it as a string. Thereplacemethods remove non unit characters that could be present in the string.The goal here is to breakup the unit into components to check if one of them is DN. Just checking for "DN" in the unit string is ambiguous because it's possible some unit could include a contiguous "dn" in it somewhere.
Ah!
I don't like this approach.
I do.
What kind of strings do we have to handle?
If I can see some examples, we could hopefully come up with another method.
There was a problem hiding this comment.
See the updated _parse_fits_unit tests for some examples.
There was a problem hiding this comment.
Rather than this abomination why not just do this:
unit = u.Unit(unit_str)
if u.DN not in unit.bases:
warn_metadata(...)
unit = None
Fixes #6566