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

update moment map plugin to use correct display flux units for moment 0 #2877

Merged
merged 1 commit into from
May 28, 2024

Conversation

cshanahan1
Copy link
Contributor

@cshanahan1 cshanahan1 commented May 16, 2024

This PR updates the moment map plugin to reflect if the current display unit is a Flux or Surface Brightness unit for moment 0 calculations.

@github-actions github-actions bot added cubeviz specviz plugin Label for plugins common to multiple configurations labels May 16, 2024
@cshanahan1 cshanahan1 marked this pull request as ready for review May 17, 2024 18:22
@javerbukh
Copy link
Contributor

If I create a moment map in the cubeviz example notebook, then convert to Jy/sr and overwrite the first moment map, I still see MJy/sr in the coords display. The moment map plugin itself looks correct and has the correct units, so if updating coords info is out of scope this is looking good! The one maybe bug I found is that when you first open the moment map plugin, there are no output units selected. If you press calculate, however, it still gives you a result.

@cshanahan1 cshanahan1 modified the milestones: 3.10.2, 4.0 May 20, 2024
@kecnry
Copy link
Member

kecnry commented May 20, 2024

I think the unit stuff needed for this won't be released until 4.0, so this probably won't need to be backported or milestoned to 3.10.2 - is that right?

Copy link

codecov bot commented May 23, 2024

Codecov Report

Attention: Patch coverage is 90.47619% with 4 lines in your changes missing coverage. Please review.

Project coverage is 88.70%. Comparing base (8f65073) to head (4160c62).
Report is 176 commits behind head on main.

Files Patch % Lines
...configs/cubeviz/plugins/moment_maps/moment_maps.py 88.46% 3 Missing ⚠️
jdaviz/core/validunits.py 93.75% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2877   +/-   ##
=======================================
  Coverage   88.70%   88.70%           
=======================================
  Files         111      111           
  Lines       17086    17123   +37     
=======================================
+ Hits        15156    15189   +33     
- Misses       1930     1934    +4     

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

@cshanahan1
Copy link
Contributor Author

@javerbukh I both of those concerns - the non-selection still allowing you to calculate a MM is now fixed, and the units of the displayed and / or saved MM should reflect the correct display units now.

Copy link
Contributor

@javerbukh javerbukh left a comment

Choose a reason for hiding this comment

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

Code looks good and the points I made earlier have been addressed. Nice work!

Copy link
Contributor

@gibsongreen gibsongreen left a comment

Choose a reason for hiding this comment

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

Everything tests out and looks good to me!

@cshanahan1
Copy link
Contributor Author

thanks for the reviews, i just rebased and ill merge this once tests pass

@cshanahan1 cshanahan1 merged commit ec80391 into spacetelescope:main May 28, 2024
17 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
cubeviz plugin Label for plugins common to multiple configurations specviz
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants