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

Fix units JSON units error in notebook #3206

Merged
merged 18 commits into from
Oct 4, 2024

Conversation

cshanahan1
Copy link
Contributor

@cshanahan1 cshanahan1 commented Sep 30, 2024

This PR fixes the issue described in JDAT-4817

Some code cleanup in the unit conversion plugin is also in this PR. Setting available flux units for unit conversion based on the loaded dataset was able to be simplified. This includes:

  • renamed what was sometimes referred to as 'locally defined flux units' to 'spectral_and_photon_flux_density_units', for clarity since this list is used around the app. These are the list of all units that are convertible to one another and will be provided if the loaded data is equivalent to these units (with or without u.spectral_density equivalency)
  • Simplified the logic for setting flux unit options in the dropdown. I removed a check for additional equivalencies using 'flux_unit.find_equivalent_units()'. What this was doing was checking if there were any additional equivalencies for an input unit (via astropy units code). However, since it was specified not to include prefixes, it would only return Jy, AB for data loaded in Jy, and then magnitude units were being manually removed from the list, this was not doing anything meaningful. Additional flux unit options are only populated if the unit is equivalent to a spectral or photon flux density unit, with or without u.spectral_density equivalency. Otherwise, the only item in the dropdown should be the loaded unit (e.g if 'counts' loaded, only 'counts' in the dropdown for flux unit). Units that are equivalent but with a different prefix (e.g nJy) will have the options in spectral_and_photon_flux_density_units available for conversion, as well as the native unit at the bottom of the list. If we think it is realistic that some unit could be loaded, that would be detected as the 'flux' unit in a composite unit and have additional equivalencies returned by flux_unit.find_equivalent_units(), then I could add this back in, but I can't think of a case and none of these cases are in tests or elsewhere in the code. It makes sense to me to either give no additional options, or options that are flux densities if the input is a flux density
  • All setting of dropdown options for flux/angle are handled within their respective functions in validunits.py, moved out of unit_conversion.py

Test coverage for unit conversion in cubeviz and specviz is added - the new tests test that for a cube loaded in a given unit, the dropdown items for available flux units to convert to in the unit conversion plugin are as expected.

@github-actions github-actions bot added cubeviz specviz testing imviz plugin Label for plugins common to multiple configurations labels Sep 30, 2024
@cshanahan1 cshanahan1 changed the title Fix units JSON error in notebook Fix units JSON units error in notebook Sep 30, 2024
@cshanahan1 cshanahan1 added this to the 4.0 milestone Sep 30, 2024
Copy link

codecov bot commented Sep 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.49%. Comparing base (f880d55) to head (c330a93).
Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3206      +/-   ##
==========================================
+ Coverage   88.46%   88.49%   +0.02%     
==========================================
  Files         125      125              
  Lines       18668    18671       +3     
==========================================
+ Hits        16514    16522       +8     
+ Misses       2154     2149       -5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jdaviz/core/validunits.py Outdated Show resolved Hide resolved
jdaviz/core/validunits.py Outdated Show resolved Hide resolved
Copy link
Member

@kecnry kecnry left a comment

Choose a reason for hiding this comment

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

thanks for the cleanup and added test-coverage - just a few small questions/comments

@pllim
Copy link
Contributor

pllim commented Oct 3, 2024

Looks like you are still pushing changes, let me know when all the changes are done. So far I like the simplification. Thanks!

@cshanahan1
Copy link
Contributor Author

@pllim just responding to review comments - i've responded to all as of now so feel free to keep reviewing

jdaviz/core/custom_units.py Outdated Show resolved Hide resolved
Copy link
Contributor

@pllim pllim left a comment

Choose a reason for hiding this comment

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

Except the typo I introduce, code LGTM. Thanks!

@cshanahan1 cshanahan1 merged commit 12d3b04 into spacetelescope:main Oct 4, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cubeviz imviz plugin Label for plugins common to multiple configurations Ready for final review specviz testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants