-
Notifications
You must be signed in to change notification settings - Fork 74
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
Units cleanup #3204
Units cleanup #3204
Conversation
cshanahan1
commented
Sep 26, 2024
•
edited
Loading
edited
- Defined a pixel squared unit PIX2 and replaced instances of u.pix * u.pix with PIX2 across the code
- 'locally defined flux units', which is the list of flux units that an input unit is able to be converted to if it is compatible with units in the list with or without u.spectral_density, is now defined in one place only. (I believe the 'create_flux_equivalencies_list' can be further simplified and just use this list as well as the input unit if it is equivalent to a unit in this list, but i did not address that in this PR.)
- some functions added to combine flux and angle strings to get a properly formatted surface brightness unit, used to define the list of all available spectral y axis units
- moved cubeviz unit conversion tests to their own file, out of specviz tests
- added a skipped test that should pass after JDAT 4785 is resolved (@pllim). It was a missing but essential test to test all cubeviz unit conversions that are in the dropdown when a cube is loaded in Jy.
cf145de
to
b8d5a82
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3204 +/- ##
=======================================
Coverage 88.45% 88.46%
=======================================
Files 124 125 +1
Lines 18649 18668 +19
=======================================
+ Hits 16496 16514 +18
- Misses 2153 2154 +1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great, thanks for doing this! Just a few small questions from skimming through the diff
'ph / (Angstrom s cm2 pix2)', 'ph / (Hz s cm2 pix2)', | ||
'ct / pix2' | ||
] | ||
all_flux_units = locally_defined_flux_units() + ['ct'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a reason not to include ct
(perhaps optionally) in the function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to keep it clear that 'locally_defined_flux_units' are units that can be converted to and from one another, and counts can not.
(This is a tangent but I wanted to write down my thoughts before I forget):- locally_defined_flux_units is the list of units that appears in the dropdown if your loaded data is compatible with one of the units in that list. A more descriptive name might make sense, especially to make it clear that this list of units is the available conversions for data loaded in a compatible unit, and is not relevant otherwise.
If your loaded data is in counts, and you wanted to be able to convert from counts to e/s (not sure if this actually makes sense, just an example), then there would be another list of 'flux units' that is ['counts', 'e/s'] etc.
Eventually we could use these lists of units we support and support conversions between to check the input against and raise warnings if you load a nonsense unit. We could also do away with the 'create_flux_equivalencies_list` function and just check if the input unit is compatible with one of these 'locally_defined_flux_units' lists, and append the input unit as a choice if it is (and if not, there will be no other choices for flux conversion so this will avoid the issue where it provides those units and the conversion breaks)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, thanks for the explanation. The flux choices in unit conversion is still populated by create_flux_equivalencies_list
- should that be changed here/now or not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its going to require a little more thought and writing some more tests to make sure different loaded units get the correct unit choices, so maybe not in this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good - deal!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there anything new/changed here or are these just moved from test_unit_conversion
? (Seems to be just a move from your PR description, but wanted to double check in case there is something we should actually look at closer in review)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a new (skipped for now until contours bug is fixed) test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks! I always appreciate some code cleanup.