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

update y axis label based on unit type #2703

Merged
merged 2 commits into from
Feb 14, 2024

Conversation

cshanahan1
Copy link
Contributor

Check unit type (e.g flux density, surface brightness, counts, etc) for generating display label for the y axis in spectral viewer. Previously it was hard coded to always display 'flux density' no matter the input unit.

Closes #2701

elif y_unit.is_equivalent(u.erg / (u.s * u.cm**2 * u.Hz)):
flux_unit_type = 'Flux'
elif y_unit.is_equivalent(u.e / u.s):
flux_unit_type = "Counts"
Copy link
Contributor

Choose a reason for hiding this comment

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

"counts" is very ambiguous by STScI standard. I don't think this is correct.

Copy link
Member

Choose a reason for hiding this comment

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

given that the unit is also in the label, I think this is probably ok?

Copy link

codecov bot commented Feb 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (fb6d841) 90.83% compared to head (04a29ed) 90.84%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2703   +/-   ##
=======================================
  Coverage   90.83%   90.84%           
=======================================
  Files         163      164    +1     
  Lines       21502    21524   +22     
=======================================
+ Hits        19532    19553   +21     
- Misses       1970     1971    +1     

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

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.

I think it is also a matter of how consistent we want to be w.r.t. y_unit.physical_type that you get from astropy.units. If consistency isn't a concern, then what you have is fine.

CHANGES.rst Outdated Show resolved Hide resolved
jdaviz/configs/specviz/plugins/viewers.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.

Do we need tests?

@pllim
Copy link
Contributor

pllim commented Feb 13, 2024

I think you can access self.figure.axes[1].label and check that the desired flux unit type is there, no?

@cshanahan1
Copy link
Contributor Author

added tests

@cshanahan1 cshanahan1 added this to the 3.9 milestone Feb 13, 2024
CHANGES.rst Outdated Show resolved Hide resolved
@pllim
Copy link
Contributor

pllim commented Feb 13, 2024

Thanks! Won't catch edge cases like u.electron (without per second) but we can handle those as need arises because there are unlimited combos possible.

CHANGES.rst Outdated Show resolved Hide resolved
@pllim
Copy link
Contributor

pllim commented Feb 13, 2024

devdeps failure is unrelated (glue-viz/glue-jupyter#421)

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.

Definitely an improvement and we can always iterate or move to somewhere more general (for all axes in all viewers) down the road. Thanks for finding and fixing this!

jdaviz/configs/specviz/plugins/viewers.py Outdated Show resolved Hide resolved
Co-authored-by: P. L. Lim <[email protected]>

Update jdaviz/configs/specviz/plugins/viewers.py

Co-authored-by: P. L. Lim <[email protected]>
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.

LGTM. Thanks!

@pllim pllim merged commit c8c45db into spacetelescope:main Feb 14, 2024
14 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update Y axis label on spectrum viewer to accurately describe unit type
3 participants