-
Notifications
You must be signed in to change notification settings - Fork 74
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
Cubeviz spectral extraction through plugin #2827
Cubeviz spectral extraction through plugin #2827
Conversation
dcc95f2
to
b378849
Compare
* cannot truly deprecate this since the functionality is changing, but we'll leave a user-API entry that raises an explanatory error in its place
* and use the default label logic for the automatic full cube extraction (no longer including "Entire Cube" in the label)
* now that this is required functionality
* remove unneeded code for suffix * hide spatial subset from spectrum-viewer's legend
I got a traceback from this branch. Workflow to reproduce:
I get a warning saying that spectrum tied to Subset 3 is removed, but I still see Subset 3 on image viewers and Subsets drop-down. When I open the log in Lab, I see this traceback:
|
@pllim The version of the example notebook I'm using does not create 4 subsets. Do you have the latest version? |
@javerbukh , run all the cells. The second half of the notebook loads regions into the app from API. |
Specifically this one: # photutils aperture
my_aper = CircularAperture((15, 10), r=5)
# regions shape
my_reg = CirclePixelRegion(center=PixCoord(x=35, y=35), radius=10)
my_regions = [my_aper, my_reg]
cubeviz.load_regions(my_regions) |
I only get 2 subsets when running all the cells. |
I can reproduce with just 2 manually created spatial subsets (but oddly not with a single spatial subset, which was previously my test case, and also oddly seems to be isolated to the last added subset). I'll look into and try to fix it - hopefully shouldn't hold up continuing reviews in the meantime! 🤞 |
Oh oops... sorry, I forgot I also drawn 2 with my hands before that. |
I found another subset related bug (so sorry) and confirmed it is not on main:
I see a
|
Looks like the previously mentioned bug is only in cubeviz and not specviz. I also confirmed that this PR has the same behavior in other configs as on main, although I did notice that subsets are apparently not working in mosviz. I'll check if there is already a ticket for that and if not, create one. |
The "re-parenting" traceback when deleting the last-created (but not first) subset, should now be fixed. |
before the assumption of data_collection[0] was the cube and therefore the "parent" of the spectral subset, but that is no longer the case. This solution isn't perfect, but isn't any worse than the previous assumption
This one should also now be fixed. |
Can confirm that the bug I was seeing is fixed, thanks! |
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.
Let's get it in then before Wednesday. Please don't forget the change log. Thanks!
This is now combined with #2781 and I tried to add changelog entries that would cover the breaking changes without getting too verbose. If anyone has comments on that, let me know, otherwise I can merge once CI is green. |
a0d6666
to
ad8dbf3
Compare
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.
LGTM.
Is there any follow-up needed for all the JDAT notebooks out there?
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'm not sure, I did go through and update the example notebooks. I'll create a general ticket for updating JDAT for any/all breaking changes in 4.0 - that's a good idea.
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.
Description
This pull request replaces the glue automatic cube collapse with the spectral extraction plugin and the auto-updating plugin results functionality introduced in #2680. This is being done both for internal self-consistency but also to properly handle different resulting units depending on the function (which can then be sent through #2781 to "translate" between surface brightness and flux, as needed).
Updated cubeviz export_data docs
auto/updating-extraction
Extracted spectrum is now a separate data entry from the flux cube. Creating a spatial subset automatically runs through the spectral extraction plugin, resulting in an auto-updating data product.
Screen.Recording.2024-05-02.at.2.28.39.PM.mov
gaussian smooth
If spectral smooth is selected, then the output shape depends on the input (cubes result in cubes, with NO automatic extraction, spectra result in spectra). Spatial smoothing always results in a cube (with NO automatic extraction).
Screen.Recording.2024-05-02.at.2.27.31.PM.mov
model fitting
Cube fitting option is moved to the top and changes the available options for input data. Spatial subset selection is gone in place of choosing the auto-extracted spectrum. Model parameter values for cubes are estimated based on an on-the-fly extraction using
mean
.Screen.Recording.2024-05-02.at.2.31.57.PM.mov
line analysis
Spatial subset selection is gone in place of choosing the auto-extracted spectrum.
Screen.Recording.2024-05-02.at.2.32.48.PM.mov
moment maps
Continuum subtraction now allows to choose which existing 1d spectrum should be used to visualize the continuum inputs. Note that the continuum is ultimately computed with respect to the full input cube.
Screen.Recording.2024-05-02.at.2.35.11.PM.mov
This PR accomplishes this by doing the following:
is_flux_cube
filter (so that model fitting can make use of collapsing functionality for smoothed cubes, etc)continuum_dataset
dropdown to select which 1d spectrum to use for determining the continuum (for visualization only). Note this is only shown when more than one option exist - so we could add additional logic to filter the choices to match the input cube and with no spatial subsets as input, in which case multiple entries would only show if there are multiple manual extractions which might then be useful for the user to switch between. Or we could hide this entirely from the user and just take the first match (effectively the same as previous behavior).deprecatesREMOVES SUPPORT forfunction
andspatial_subset
kwargs toget_data
to get a spectrum from a cube.spatial_subset
can still be passed in cubeviz/imviz to get a masked cube/image, but spectral extraction must now go through the dedicated plugin.This might qualify for a major version bump (4.0 instead of 3.11 milestone)5029181714403517141311630 broken 😭)TODO:
jdaviz/jdaviz/core/helpers.py:586: UserWarning: Not able to get Spectrum (sum) returned with subset Subset 1 applied of type <class 'specutils.spectra.spectrum1d.Spectrum1D'>.
cubeviz.specviz.get_spectra()
works with spatial and/or spectral subsets addedparentElement
warnings in dev consoleChange log entry
CHANGES.rst
? If you want to avoid merge conflicts,list the proposed change log here for review and add to
CHANGES.rst
before merge. If no, maintainershould add a
no-changelog-entry-needed
label.Checklist for package maintainer(s)
This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.
trivial
label.