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

Do not replace "DN" with "ct" in Map.unit #7544

Closed
wtbarnes opened this issue Apr 2, 2024 · 2 comments
Closed

Do not replace "DN" with "ct" in Map.unit #7544

wtbarnes opened this issue Apr 2, 2024 · 2 comments
Labels
Discussion An issue opened for, or undergoing discussion. io Affects the io submodule map Affects the map submodule Priority Low Slow action required

Comments

@wtbarnes
Copy link
Member

wtbarnes commented Apr 2, 2024

Provide a general description of the issue or problem.

Currently, when parsing the unit from a Map, we replace any use of "DN" in Map.unit with "ct"/"count",

sunpy/sunpy/map/mapbase.py

Lines 727 to 742 in 0886efa

def _parse_fits_unit(unit_str):
replacements = {'gauss': 'G',
'dn': 'ct',
'dn/s': 'ct/s',
'counts / pixel': 'ct/pix',}
if unit_str.lower() in replacements:
unit_str = replacements[unit_str.lower()]
unit = u.Unit(unit_str, format='fits', parse_strict='silent')
if isinstance(unit, u.UnrecognizedUnit):
warn_metadata(f'Could not parse unit string "{unit_str}" as a valid FITS unit.\n'
f'See {_META_FIX_URL} for how to fix metadata before loading it '
'with sunpy.map.Map.\n'
'See https://fits.gsfc.nasa.gov/fits_standard.html for '
'the FITS unit standards.')
unit = None
return unit

The logic here is that "DN" is not a recognized unit in the FITS standard so we shouldn't allow it as a valid value for the BUNIT key. However, I think there are three reasons we should NOT be making this replacement:

  1. We currently only cover a subset of the cases here, e.g. "DN/pix/s" results in the unit key not being parsed at all. It seems impractical to continue to try to cover all possible uses of "DN".
  2. The use of "count"/"ct" is ambiguous and a confusing substitute for "DN". This can sometimes mean "electrons" or "photons", but is rarely used to denote "DN". Furthermore "DN" is commonly used throughout solar physics and it is confusing to not use this convention.
  3. astropy.io.fits can write and read files with "DN" in the BUNIT key without issue.
@wtbarnes wtbarnes added map Affects the map submodule io Affects the io submodule Discussion An issue opened for, or undergoing discussion. Priority Low Slow action required labels Apr 2, 2024
@ayshih
Copy link
Member

ayshih commented Apr 2, 2024

Duplicate-ish of #6566

@wtbarnes
Copy link
Member Author

wtbarnes commented Apr 2, 2024

Well I clearly can't remember my own issues. I'm closing this and I've copied my points over to #6566.

@wtbarnes wtbarnes closed this as completed Apr 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discussion An issue opened for, or undergoing discussion. io Affects the io submodule map Affects the map submodule Priority Low Slow action required
Projects
None yet
Development

No branches or pull requests

2 participants