-
Notifications
You must be signed in to change notification settings - Fork 75
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
SpectralCube to specutils transition in cubeviz #547
Conversation
Latest changes can be tested by running the CubevizExample notebook to the 4th cell. Then, go to |
I am seeing this output, but no data gets loaded:
But isn't that expected, since |
That is correct about |
Latest changes have a manga cube load into cubeviz using Spectrum1D. Some hacky things had to be done for this to work, like using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's what I get when I change the axes:
which is probably not what it's supposed to look like. I spent a few hours trying different things to no avail. My only possible theory is whether us moving the axes around (to accommodate for the spectral axis) is either confusing us, or the glue backend) and causing the axes to be mislabeled. I was going to try and see if I could plot it in matplotlib to see if I understood the axes correctly, but promptly got confused...
TL;DR: App-wide conversion to Spectrum1D will take longer if wavelength axis is forced to be last, and the time it takes for data to load is significantly longer. Do we still want to continue development in this direction with that in mind? Is there any chance we can change the decision on the wavelength axis being last in Spectrum1D? @eteq The latest commit has working axes (check Another thought is that there may be work in glue-astronomy that can make this conversion simpler, but that development work is made more complicated by the "wavelength as last axis" decision, in my opinion. That said, I will proceed in this direction for now. |
@@ -31,7 +31,7 @@ install_requires = | |||
ipygoldenlayout>=0.3.0 | |||
voila>=0.2.4 | |||
pyyaml>=5.4.1 | |||
specutils>=1.4.0 | |||
specutils@git+https://github.com/astropy/specutils.git | |||
glue-astronomy>=0.3.2 | |||
click>=7.1.2 | |||
spectral-cube>=0.5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't keep track anymore. Can we completely remove spectral-cube yet, or do we have to wait for "Spiderman: Far from Home"?
spectral-cube>=0.5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Waiting for Far from Home (Spectrum1D collapse, for posterity) unfortunately!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
p.s. I am basically a co-author by now, so I'll abstain from approving.
@PatrickOgle I'm not sure I understand, does smoothing need to happen in multiple axes, or in a different axis? This is what I see after smoothing |
jdaviz/configs/default/plugins/gaussian_smooth/gaussian_smooth.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The dev test failure looks unrelated, so I'm approving at this point. Nice job! I would still like to see @PatrickOgle comment that the moment maps output looks reasonable before merging (especially since he caught a problem with the smoothing!).
The Gaussian spatial smoothing now appears to be working properly. I have verified the following plugin functionality:
However, Moment 2 Crashes, even though it works fine with jdaviz2.0.0... |
For this PR, Moment 2 map on either the whole spectrum or a spectral region crashes on NIRSpec IFU spectral cube processed with JWST pipeline. Moment 0 and Moment 1 work properly. Same error does not occur with jdaviz2.0.0.
|
@rosteen Is the bug @PatrickOgle found in |
Filed this jdaviz issue report for Moment = 2 failure: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I previously commented, the Gaussian Smooth, Collapse and Moment Maps Plugins appear to work properly, except for an issue with Moment 2 for a NIRSpec IFU dataset.
This lien notwithstanding, the rest looks good.
Resolves #544
Requires
Specutils 811Also requires a fix to glue-astronomy 28
Edit: Requires
glue-astronomy 292nd Edit: Requires Specutils 822 and glue-astronomy 36