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.

Sign up for GitHub

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

Jump to bottom

Use of u.DN unit in place of u.count #6566

Open
wtbarnes opened this issue Nov 3, 2022 · 3 comments · May be fixed by #7585
Open

Use of u.DN unit in place of u.count #6566

wtbarnes opened this issue Nov 3, 2022 · 3 comments · May be fixed by #7585
Labels
Discussion An issue opened for, or undergoing discussion. map Affects the map submodule

Comments

@wtbarnes
Copy link
Member

wtbarnes commented Nov 3, 2022

Provide a general description of the issue or problem.

DN is a commonly used unit in solar physics and often shows up in FITS headers. Currently, we replace DN with "count" in GenericMap,

sunpy/sunpy/map/mapbase.py

Lines 722 to 726 in dbcf1b5

replacements = {'gauss': 'G',
'dn': 'ct',
'dn/s': 'ct/s'}
if unit_str.lower() in replacements:
unit_str = replacements[unit_str.lower()]

This is primarily because DN is not a unit that is recognized by the FITS standard but also because until recently, the closest representation of DN in astropy.units is u.count. However, as of astropy 4.3, there is now a u.DN: https://docs.astropy.org/en/v4.3.1/units/index.html#module-astropy.units.astrophys. This raises the question of whether we should switch to using u.DN instead of u.count once we pin to >= 4.3.

The main disadvantage to this would be that any map with units of u.DN would not be FITS serializable.

See also the discussion in the chat on this topic: https://matrix.to/#/!JYqfIVJjWANcHnfktY:cadair.com/$166743304126958mwNUw:matrix.org?via=cadair.com&via=matrix.org&via=openastronomy.org
@wtbarnes wtbarnes added map Affects the map submodule Discussion An issue opened for, or undergoing discussion. labels Nov 3, 2022
@Cadair
Copy link
Member

Cadair commented Nov 9, 2022

The main disadvantage to this would be that any map with units of u.DN would not be FITS serializable.

Is this true, or would Astropy just throw a warning?

@ayshih ayshih mentioned this issue Apr 2, 2024
Closed
@wtbarnes
Copy link
Member Author

wtbarnes commented Apr 2, 2024 •
edited

I'm pulling this from my duplicate issue #7544:

I think there are three reasons we should NOT be making this replacement of "DN" for "count":

  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
Copy link
Member Author

wtbarnes commented Apr 2, 2024

The main disadvantage to this would be that any map with units of u.DN would not be FITS serializable.

Is this true, or would Astropy just throw a warning?

I do not stand by my original statement. I believe astropy will just throw a warning.

@wtbarnes wtbarnes linked a pull request Apr 23, 2024 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Discussion An issue opened for, or undergoing discussion. map Affects the map submodule
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Allow DN as a unit on GenericMap wtbarnes/sunpy
2 participants
@Cadair @wtbarnes