-
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
Plugin: spectral extraction #1514
Conversation
3efcde2
to
87eeb5f
Compare
<v-icon style='margin-right: 2px'>{{ data.item.icon }}</v-icon> | ||
<v-icon v-if="data.item.icon" style='margin-right: 2px'>{{ data.item.icon }}</v-icon> |
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.
this just makes the left of the entries with and without icons line up better. I also applied this across all other instances, even though not all dropdowns use "manual entries".
<j-tooltip :tooltipcontent="'data label: '+item.name" span_style="font-size: 12pt; padding-top: 6px; padding-left: 6px; width: calc(100% - 140px); white-space: nowrap; cursor: default;"> | ||
<j-tooltip :tooltipcontent="'data label: '+item.name" span_style="font-size: 12pt; padding-top: 6px; padding-left: 6px; width: calc(100% - 80px); white-space: nowrap; cursor: default;"> |
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.
this fixes an alignment issue with the "x" button. #1491 apparently didn't properly set the alignment for all cases.
state_filter=is_profile) | ||
state_filter=not_image) |
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.
Lines used to just be spectra, but now we have a line that is in an image viewer and needs to expose the right plot options.
@@ -1018,8 +1021,27 @@ class DatasetSelect(BaseSelectPluginComponent): | |||
""" | |||
def __init__(self, plugin, items, selected, | |||
filters=['not_from_plugin_model_fitting', 'layer_in_viewers'], | |||
default_text=None, manual_options=[], |
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.
We didn't have a use for manual options in dataset dropdowns until now, so they weren't exposed. This works the same way as all other Select components that inherit from BaseSelectPluginComponent.
replace = self.viewer.selected_reference != 'spectrum-viewer' | ||
if replace is None: | ||
replace = self.viewer.selected_reference != 'spectrum-viewer' |
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.
replace used to be hardcoded to True for viewers other than spectrum-viewer, but in this new plugin, we definitely want to plot the trace on top of the existing image, not replacing it.
@@ -34,6 +34,7 @@ install_requires = | |||
voila>=0.3.5 | |||
pyyaml>=5.4.1 | |||
specutils>=1.7.0 | |||
specreduce>=1.0.0 |
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.
specreduce will be an added dependency (although we might need to pin a newer version).
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.
Yet another required dependency... Is @havok2063 okay with this?
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.
If not, we could have it be required for the plugin to show up, but then users would need to install additional dependencies depending on which plugins they want.
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.
Tricky business: #1399
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 think I'm ok with it, since we will use this plugin in MAST. Does this plugin also provide users information on the default spectral extraction that occurs when I load a single s2d file? Or is it only for performing custom extractions?
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 far as I know, the (current) plan is only for custom extraction and using jdaviz as a visualization layer to existing and upcoming functionality in specreduce.
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.
User feedback said they wanted to know how the default extraction was made. If we switch the default extraction to use this plugin to perform the extraction on initial load, would that provide that info and satisfy this feedback?
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.
That's an interesting idea. I think the current displayed spectrum is just collapsed across the entire image. But if we could come up with automated defaults as input into the specreduce functionality, use those for the initial collapse instead, and have those set as the default in the plugin, that would be slick! I'm guessing even if that extraction isn't perfect, it would be better than just the full collapse?
There already is a snackbar warning that says: "The collapsed 1D spectrum is for quicklook purposes only. A robust extraction should be used for scientific use cases." Perhaps this text should also be updated to direct the user to this new plugin? Or maybe the "Spectrum 1D" data label should be changed to "Collapsed Spectrum"?
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.
Yeah I think it would be fine to define some reasonable defaults to perform a default extraction. We would still display the snackbar warning message. Yeah it's also a good idea to update the message to point people to the plugin. If it's too much work to switch over, then I am also ok just updating the label to say "Collapsed Spectrum" and updating the message to point people to the plugin.
7693b7a
to
10eee9e
Compare
@kecnry This looks really good. My biggest suggestion would be to make the trace and extraction update automatically as you change the parameters, the products should then only be written out at the end (bottom of extraction workflow), similar to other plugins. Background subtraction might be more difficult to dynamically update, since it involves a 2D rather than 1D product, but that is just my guess. |
This is definitely do-able (maybe using the same blue we do in line analysis to make a clear distinction between the live preview and actual result), but I'm a bit worried about all the things we'll want to visualize as we implement the rest of the plugin: the trace, the adopted regions for the background (some width and separation around the trace), the background image and/or subtracted image, the region adopted for the extraction (some width around the trace), and finally the extracted spectrum. Showing all of these at once in an intuitive way might be not be possible with our limited real estate. I think the regions for background and extraction might be the most important to show in real-time since they're just visualizing the inputs rather than the outputs and would really help the user to understand what the parameters mean. But which of the others do we want to show without making it too cluttered? One option might be to have the visual feedback be temporary - so that when you change options for the trace, there is some live visual "preview" feedback but that only stays on the screen for 5 seconds, or maybe stays until you make changes to one of the other steps, and then goes away so you can see feedback from the current step. I'd be happy to give this a try if people like the idea, but it'll take some framework implementation first.
I see and agree with your point that for simple cases this would be more convenient. But unless we really don't want to support more complex workflows (few cases I have in mind listed below), I'd like to try to keep support for importing/exporting at each intermediate step. What about something like this to make the simpler case the default while still having the flexibility for interacting in the UI or notebook at each step? If we wanted to simplify it even more, we could have an "simple/advanced" toggle at the top which hides the export expandable sections and all the dropdowns that default to "From Plugin" which would effectively enforce a flow from top to bottom without all the clutter. Screen.Recording.2022-07-21.at.1.07.12.PM.movCases where the more advanced workflow might be useful:
|
@PatrickOgle - this is what I've managed to come up with as a prototype from your suggestions (both for an easier linear-flow and for visual feedback) - let me know what you think! Unfortunately, I don't think this will be capable of live-preview of the background/subtracted images, but does visualize all of the various input parameters and does so in a way that isn't too cluttered (and hopefully is still intuitive - cc @Jenneh?). Screen.Recording.2022-07-21.at.4.12.00.PM.movA slightly more complex workflow where you might want to use separate traces for the background and extraction would then look something like this Screen.Recording.2022-07-21.at.4.20.33.PM.mov |
Ok, that's very useful as a reference, thanks! (1) is implemented above in the comment I posted almost exactly as you posted. |
# TODO: range(len(...)) is a temporary hack for this data displaying in meters instead of pixels | ||
self.marks['trace'].update_xy(range(len(trace.trace)), trace.trace) |
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.
will definitely want to fix this or find a workaround so that the underlying bug doesn't become a blocker to this PR 🐱
8f420b4
to
98f89a8
Compare
@havok2063 - I managed to implement your suggestion to use this as the default extraction method (if the user only loads a 2D spectrum). It currently defaults to a simple extraction with the trace set somewhat intelligently based on median of the row number of the brightest pixel in each column and with a width of 10% the full image height. When you open the plugin, you'll then see the settings that were used to extract that spectrum, and the snackbar is updated to state: "The collapsed 1D spectrum was generated automatically. Use the spectral extraction plugin for custom extraction." Screen.Recording.2022-07-22.at.2.25.29.PM.mov |
c9011d3
to
4e65f50
Compare
@kecnry That looks great! This plugin vastly improves the reasons for using Specviz2d. Can we change the snackbar message to the following? One follow-up question, if someone performs a custom extraction in the GUI, is there a GUI way to save/download it as a file. Thinking about the use case of using this in MAST. |
I thought Specviz2D was a temporarily solution until "something" happens? We don't even really document it in the user docs. Should we start? |
@@ -174,6 +174,7 @@ Using Jdaviz | |||
imviz/index.rst | |||
specviz/index.rst | |||
cubeviz/index.rst | |||
specviz2d/index.rst |
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.
Need to add specviz2d stuff to API docs too?
Also add new Specviz2D section to change log template in the release instructions?
On GitHub itself, should I create a new Specviz2D label and add corresponding Issue and Bug templates?
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.
Since specviz2d is currently used by MAST (which links to a non-existent RTD page), I think we should... and we can always merge with mosviz later if we decide to go that route. Full docs are out-of-scope for this PR, I think/hope, but I just put the bare minimum so that all the plugin links point somewhere appropriate.
I added a specviz2d label in GitHub already (for this PR) and added a custom section in the change log (but not the template). The helper was already in the docs, but I'll add this plugin as well, thanks for catching that!
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 book-keeping stuff (templates, release instructions) can be done after this is merged. 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.
I'll let @rosteen decide if anything in JIRA needs updating to include specviz2d...
* plotting the 1D spectrum in wavelength/frequency and linking to the 2D spectrum viewer can have adverse affects when the mapping is highly non-linear * previous behavior in main was to plot in pixel-space but had incorrect wavelength label/units on the x-axis (and in the accessed spectrum1d object). This PR will partly fix that by being consistent with units, but no longer attempts to plot in actual wavelengths until a more general solution is implemented.
* UserWarning from traitlets was being caused by setting the x/y values on the bqplot line, by casting to an array, this goes away * now that extraction replaces collapse, the NAXIS meta is no longer set. As far as I can tell, this is unused so can be safely removed from the test. If that isn't the case, we can manually add to the meta-data when extracting instead.
* for now setting to False, but could see the argument that they should default to True to mimic the behavior of the UI buttons which do add data to the app
* can be re-implemented once the glue-astronomy PR is merged and released
Co-authored-by: Ricky O'Steen <39831871+rosteen@users.noreply.github.com>
* could cause confusion with the actual spectral line in the 2d spectrum viewer
* the "accordion" container and the mouseover text includes the necessary context
876a9b2
to
c42f1af
Compare
Rebased on top of #1538 which should fix the bug found by @rosteen and @javerbukh. |
I think this looks good to me now, thanks for addressing those comments. |
I feel like @ojustino should give the second approval since he has been very involved in specreduce. |
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.
Looks good to me, thanks for addressing all the feedback! I'm OK with @ojustino at least taking a look at this monster of a PR before merging 😄
Even though it uses |
Thanks for the thorough reviews! @ojustino - I'll go ahead and merge then - I know the plan is to continue to iterate on this, so whenever you do get a chance to try it, feel free to recommend or make any changes. |
Description
This pull request implements a new plugin in specviz2d for spectral extraction via specreduce.
It currently only implements the functionality for Trace creation, offsetting, loading from the API, and visualizing in the 2d spectrum viewer. The UI elements for the other "steps" (background, extraction) are implemented (in order to plan for the necessary interaction between the steps), but are currently non-functional.(woops! - that was a nice plan, but planning for how things would interact with each other eventually became just an API call away from actually implementing and devs decided it would be easier to review/merge in a complete form anyways)IMPORTANT: this PR fixes the units on the x-axis to be shown in pixels instead of erroneously in meters, but by doing so disables the line list and line analysis plugins when in pixels (whenever not manually loading a 1D spectrum in other units). Future work will either need to properly handle plotting in wavelength or update the logic in these plugins to handle converting on-the-fly using the wavelength solution, if available.
view specviz2d barebones docs and the spectral-extraction docs (full specviz2d docs are out-of-scope, but created the plugins page so links from jdaviz work).
NOTE this PR currently requires the following non-merged branches from other dependencies (and will likely fail CI tests until they are merged and pinned here):glue-astronomy#72(functionality stripped from this PR)specreduce#115(functionality stripped from this PR)Once the upstream PRs above are released, we can re-introduce the functionality in this plugin as a follow-up PR.
Sorry dev-reviewers for another large diff 😬 ! But many of the new lines are just boilerplate new plugin and boilerplate docs for specviz2d, so hopefully it won't be too difficult to parse 🤞
Note: this screen recording was still in pixel space. This has since been updated to show in wavelength, when possible, but the linking from the example notebook is so non-linear that it's almost unusable. We should probably consider switching it out with data that works better and then open follow-up efforts to improve support for more general input data.Screen.Recording.2022-07-25.at.1.43.13.PM.mov
In addition to the UI, once glue-astronomy#72 is released, we can implement support for importing traces created in the notebook via
app.add_data
(this functionality has been stripped from the PR so that we're not blocked waiting for that PR to be approved, merged, and released):You can also use the plugin API to interact with specreduce directly with something like:
Note that if the live-preview of the extracted spectrum disappears, that is because an error is being raised by specreduce. When using auto (Kosmos) trace or a manual background, specreduce is often throwing a (probably incorrect) error detecting an overlap in background regions. These cases also result in increased lag as they take a while to compute through specreduce. These cases should be fixed in specreduce and are out-of-scope here.
TODO:
range(len(trace.trace))
instances.If not removing background/extraction steps into a separate follow-up PR:
Potential Future Work:
advanced mode toggle (when in simple, hide intermediate export expansion panels as well as all inputs that default to "From Plugin")(ended up partially simplifying the options without losing too much flexibility, a separate toggle at this point is probably overkill)AddResults
component itself.Things brought up in other issues/tickets that are not (yet) directly addressed:
spectrum_result= spec2d.extract_spectrum(2dspectrum_background_subtracted_already, trace_object, extractor_object)
Post-merge TODO
Closes #809
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.CHANGES.rst
?