spacetelescope / jdaviz

JWST astronomical data analysis tools in the Jupyter platform
https://jdaviz.readthedocs.io/
BSD 3-Clause "New" or "Revised" License
138 stars 73 forks source link

fix var names in aper phot, remove specutils version check #3187

Closed cshanahan1 closed 3 weeks ago

cshanahan1 commented 3 weeks ago

This PR changes some of the unit conversion related variable names in aperture photometry to make it clearer that the spectral y axis unit is not being used for the plugin's display unit, but rather the dataset unit type (flux or sb) along with the choice of flux and angle units from the UC plugin for display. This same change needs to be made in moment maps, but since much of the code where these variables are being referenced go away in #3156, that will be addressed there. This also removes a remnant of checking for old specutils version in moment maps which can go away now that the minimum version is 1.16

cshanahan1 commented 3 weeks ago

Why does this look so familiar? Is this one of your accidentally reverted changes?

this changes the same variable names changed in #3178, and includes a specutils thing i forgot to remove in #3184

rosteen commented 3 weeks ago

What was the reason for using display_unit vs just reverting to the previous display_flux_or_sb_unit? I don't mind it, just curious.

cshanahan1 commented 3 weeks ago

What was the reason for using display_unit vs just reverting to the previous display_flux_or_sb_unit? I don't mind it, just curious.

My reasoning was that once all images in imviz are surface brightness, then the name would have to be changed, but if it is just 'display_unit' then it wouldn't have to change.

Kyle made the suggestion to eventually change it to 'sb_display_unit' though so that will happen, so if it has to change again anyway and you like flux_or_sb better i can do that.

codecov[bot] commented 3 weeks ago

Codecov Report

Attention: Patch coverage is 94.73684% with 1 line in your changes missing coverage. Please review.

Project coverage is 88.45%. Comparing base (deaab79) to head (9b3c582). Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...imviz/plugins/aper_phot_simple/aper_phot_simple.py 94.44% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #3187 +/- ## ======================================= Coverage 88.45% 88.45% ======================================= Files 124 124 Lines 18391 18389 -2 ======================================= - Hits 16267 16266 -1 + Misses 2124 2123 -1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

rosteen commented 3 weeks ago

What was the reason for using display_unit vs just reverting to the previous display_flux_or_sb_unit? I don't mind it, just curious.

My reasoning was that once all images in imviz are surface brightness, then the name would have to be changed, but if it is just 'display_unit' then it wouldn't have to change.

Kyle made the suggestion to eventually change it to 'sb_display_unit' though so that will happen, so if it has to change again anyway and you like flux_or_sb better i can do that.

I think this is fine, thanks for the explanation.