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 unit handling in continuum and moment calculations #3216

Merged
merged 3 commits into from
Oct 16, 2024

Conversation

rosteen
Copy link
Collaborator

@rosteen rosteen commented Oct 8, 2024

Fixes calculating moment 0 when display unit is in Hz, and more generally uses display unit more consistently in all continuum calculations.

@rosteen rosteen added bug Something isn't working cubeviz plugin Label for plugins common to multiple configurations labels Oct 8, 2024
@rosteen rosteen added this to the 4.0 milestone Oct 8, 2024
Comment on lines -2973 to +2974
full_spectrum = self.app._jdaviz_helper.get_data(self.dataset.selected)
full_spectrum = self.app._jdaviz_helper.get_data(self.dataset.selected,
use_display_units=True)
Copy link
Member

Choose a reason for hiding this comment

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

I feel like @cshanahan1 had just removed this. Was there a reason for that or did it just not seem to affect that use-case (see https://github.com/spacetelescope/jdaviz/pull/3211/files#r1787615549)

Copy link

codecov bot commented Oct 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.56%. Comparing base (99baa91) to head (7911b26).
Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3216   +/-   ##
=======================================
  Coverage   88.56%   88.56%           
=======================================
  Files         125      125           
  Lines       18751    18755    +4     
=======================================
+ Hits        16606    16610    +4     
  Misses       2145     2145           

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

@gibsongreen
Copy link
Contributor

In testing I am still seeing the traceback occur for moment zero (and it is occurring if you chose any spectral unit we allow).

@rosteen
Copy link
Collaborator Author

rosteen commented Oct 15, 2024

In testing I am still seeing the traceback occur for moment zero (and it is occurring if you chose any spectral unit we allow).

Can you give some more detail about your workflow? I see everything working fine on my end:

Screen.Recording.2024-10-15.at.9.53.42.AM.mov

@gibsongreen
Copy link
Contributor

I went through the workflow you showed in the video, and in the video below, and I double checked in a second conda environment and in both I was seeing the traceback:

Screen.Recording.2024-10-15.at.10.09.17.AM.mov

@rosteen
Copy link
Collaborator Author

rosteen commented Oct 15, 2024

Oh, that's not the same workflow as I used, you don't have a spectral region or continuum selected. I can't reproduce the error you're seeing though even without a region or continuum, although I do see an IndexError if I select a spectral region but have None for continuum in Hz. So I'll at least start looking into that.

@rosteen
Copy link
Collaborator Author

rosteen commented Oct 15, 2024

Ok, I just tried in a fresh environment and still only saw the error with the subset + no continuum combo, so I'll work on fixing that for now.

@rosteen
Copy link
Collaborator Author

rosteen commented Oct 15, 2024

Turns out the error I was seeing is due to a bug in specutils, see astropy/specutils#1187. We'll need to update the specutils pin after I release 1.18.0, I can do that here before we merge this.

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.

The tracebacks I was seeing came from my end, all resolved and working now! With the workflow you provided I tried many combinations with the example notebook and with a flux cube. The main notes I had I think are all outside of scope and are being addressed in follow ups and deal with the spectral density or pix2 handling.

Copy link
Contributor

@cshanahan1 cshanahan1 left a comment

Choose a reason for hiding this comment

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

I tested this with a cube in Jy / pix2 as well as Jy / sr and both work, including the continuum preview. My only take it or leave it suggestion would be to include changing spectral units and slice value in tests to make sure that those changes are reflected in the calculation.

@rosteen
Copy link
Collaborator Author

rosteen commented Oct 16, 2024

Thanks for the reviews, I just bumped the specutils pin to the new release. I'll merge once tests pass.

@rosteen rosteen merged commit 56e1ded into spacetelescope:main Oct 16, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working cubeviz plugin Label for plugins common to multiple configurations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants