Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

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

Merged
merged 4 commits into from
Sep 9, 2024

Conversation

cshanahan1
Copy link
Contributor

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

@github-actions github-actions bot added cubeviz imviz plugin Label for plugins common to multiple configurations labels Sep 9, 2024
@cshanahan1 cshanahan1 added this to the 4.0 milestone Sep 9, 2024
CHANGES.rst 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.

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

@cshanahan1
Copy link
Contributor Author

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

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.

LGTM - once Imviz can assume SB as well, I think having the context in the variable name would be useful (ie., sb_display_unit), but for now since it's toggling between SB and flux, this makes sense.

@rosteen
Copy link
Collaborator

rosteen commented Sep 9, 2024

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

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.

Copy link

codecov bot commented Sep 9, 2024

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 105 commits behind head on main.

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

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

@rosteen
Copy link
Collaborator

rosteen commented Sep 9, 2024

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.

@cshanahan1 cshanahan1 merged commit 88ff8da into spacetelescope:main Sep 9, 2024
19 checks passed
@kecnry kecnry mentioned this pull request Sep 13, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
cubeviz imviz plugin Label for plugins common to multiple configurations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants