Add tool that shows spectrum at spaxel in cubeviz #1317
Conversation
We ran into this recently in another PR... it ended up being caused by passing a quantity object to bqplot (but might also be a quantity object to any float traitlet) |
Codecov Report
@@ Coverage Diff @@
## main #1317 +/- ##
==========================================
+ Coverage 84.68% 84.73% +0.05%
==========================================
Files 91 91
Lines 7887 7987 +100
==========================================
+ Hits 6679 6768 +89
- Misses 1208 1219 +11
Continue to review full report at Codecov.
|
javerbukh
force-pushed
the
spectra-per-spaxel
branch
from
1028f7f
to
e21ad5f
Compare
javerbukh
force-pushed
the
spectra-per-spaxel
branch
from
e21ad5f
to
85792b9
Compare
javerbukh
requested review from
duytnguyendtn,
rosteen,
ojustino and
kecnry
as code owners
There was a problem hiding this comment.
I see how this could be quite handy! Just a few thoughts/questions:
- Should showing the magenta per-pixel spectrum hide all other spectral entries? Sometimes they overlap and you just see vertical lines from the collapsed full-cube spectrum, which can be confusing.
- If deactivating the tool resets the zoom on the spectrum viewer, should it also remove the magenta spectrum? Or is the user responsible for finding the entry in the data dropdown and removing it manually. We need some way to "reset", but by doing so when deactivating the tool, it's impossible to use in conjunction with zoom (to zoom in to see what pixel you selected, for example). Instead, you'd have to make sure to zoom first, and then select the pixel.
- Depending on the answer to the above question, should the created pixel entry be hidden from the user (in Data Dropdown Component #1313)? Otherwise they can toggle it and it loses memory of its color. Should it be available in data dropdowns for input to plugins (like model fitting)?
- What should happen when using the tool on a static image (moment map)? Right now it seems to show the highlight on the image but not draw on spectrum. Should we somehow de-activate the tool entirely if no layers in that viewer have a spectral axis?
- Should we allow the tool to be used on non-flux cubes (ivar, mask)? If so, do we need to change the "flux"-units on the spectrum-viewer?
| # Set y limits based on the new spectrum or spectra being created | ||
| if max(spec.flux.value) > new_y_max: | ||
| new_y_max = max(spec.flux.value) | ||
| if min(spec.flux.value) < new_y_min: | ||
| new_y_min = min(spec.flux.value) | ||
|
|
||
| with delay_callback(self.spectrum_viewer.state, 'y_min', 'y_max'): | ||
| self.spectrum_viewer.state.y_min, self.spectrum_viewer.state.y_max = (new_y_min, |
There was a problem hiding this comment.
what if instead of this, clicking automatically "unchecks" all other data entries in the data dropdown, resets y-limits (so an x-zoom is preserved), and reverts both the data selection and y-limits when deactivating the tool?
There was a problem hiding this comment.
The tool is supposed to act similarly to a subset, so deselecting data seems counter to that instruction. If the POs wanted to go in this direction however I can definitely implement that.
This is a good point and something I wanted to do originally, but the POs asked for the functionality to mirror a subset. Since a subset can be visible with its reference data, I do the same here. It makes more sense when the collapse function in the spectrum viewer is something other than sum.
I guess it depends if a user would want to do continued analysis on the spectrum after it has been created. If yes, then it should remain even after the tool has been deactivated. As for using in conjunction with the zoom tool, you bring up a good point. I'll have to think about that.
That is a good question, I think I would have to defer to the POs on that one.
The highlighted pixel should not appear in that case, I'll fix that in the next commit.
The tool already can be used on those cubes. I don't know how much unit conversion I want to get into in this PR but that is a good point. I'll have to think about that and get some feedback from the POs. |
Ok. This will have some consideration as well for #1313 - right now that allows data entries to be deleted only if meta['Plugin'] is populated. We can either edit that logic to detect some of the meta-data you set here, or maybe populate meta['Plugin'] anyways (it sort of is a plugin, it just isn't in the tray). What do you think?
Then this is different than the current subset behavior which only collapses subsets on the flux cube. We could only add the tool to the left-most viewer, but we currently have no logic that ensures that viewer has the flux cube (and that no other viewers do). Maybe this should be left as-is for now and having data-aware availability of tools should be a follow-up effort (there are other similar situations: the line selection tool in the spectrum viewer should only be available if lines are plotted, etc)? |
|
Yeah I can set that meta data, good idea. Maybe we can check if the cube has flux units? I'm not sure if that is the case for the IVAR cube but I can check that as well. |
There was a problem hiding this comment.
Not sure either of these should block this PR, so I'm approving, but might be things we want to consider for follow-up efforts and as the new tool starts to get used in practice.
-
I am torn whether the magenta box on the image viewer should stay persistent when disabling the tool. Maybe it should be tied to the spectrum data entry (so the only way to get rid of the box would be to delete that data entry)... but I can also see how that might get annoying and in the way and require an extra click. This might just take us some using this tool in practice and then revise the behavior.
-
The magenta box seems slightly offset from the image itself, but I'm unsure if its the image displaying, box displaying, or higher upstream that is to blame. See screenshot below. This is really only apparent if you zoom significantly and its still clear which pixel is highlighted, so I don't think its a major concern.
|
Re: Magenta box -- Why not make things simpler and draw a "X" at the center of the spaxel ? |
|
@pllim I already had code for a box 😄 if the team prefers an "X" I can change it. |
|
Slightly offcenter X is less obvious than a misaligned box. Just sayin'. 😉 |
|
Honestly, I think its the image that is misaligned, not the box... (look at the half-pixel marks on the axes compared to where the edges of the spaxels are drawn). |
javerbukh
force-pushed
the
spectra-per-spaxel
branch
from
5be76d1
to
f9f0537
Compare
* otherwise the toolbar collapses when opening the plugin tray and have a tool in the "selection tool" slot.
|
Should not rescale spectrum viewer on each pixel click. This destroys the context of whether the selected pixel is brighter or fainter than the previously selected one and the reference spectrum. |
Changing the function depending on the tool does not feel intuitive. Reverting the default function can be a separate ticket since it sounds like the POs need to have a discussion about that.
My only counter to this is that plugins create data that is made visible in the viewer and then can only be deselected by the user doing into the data dropdown and doing it manually. For example, this is how the model fitting plugin works.
As far as I know this is not yet implemented. @kecnry do you know how difficult that would be to implement?
I think this is intended behavior. The same interaction happens with all other tools.
If a user has their collapse function set to There is no way to compare the current pixel's spectrum to a previous one. When another pixel is clicked, the spectrum is replaced by the spectrum at the new pixel. I could see a shift-click paradigm for highlighting multiple pixels and showing the spectrum at each point, but I think that would be an enhancement to this feature. I will be out starting next Tuesday. If some of these suggestions can be broken out in to other tickets so they can be discussed and planned that would be great. It sounds like EDIT: Draft PR to show functionality from (2) and (5) is #1336 |
|
Some random thoughts...
Instead of selecting multiple tools, we can also tie this spaxel selection to a key binding, not unlike Imviz line profile. That is a completely separate operation from tools and is "always on" (can be good or bad).
Maybe it is good enough to have an API to allow you to extract the pixel's spectrum at the desired location out as |
|
I'll just comment about "tool multiselect" - that is intentionally not implemented. I suppose we could have one tool handle drag events and the other click events, but the distinction between those two is not always clear and I think it would be confusing for the user (and for the underlying infrastructure). The only idea I have if the "x" is tied to the created spectrum data-entry, and so shows up in the data-dropdown of the image viewer as well and is rendered as an "x" at that pixel because of the coordinates in the meta-data. That way, clicking on another tool (like plot options or zoom) and then returning to the pixel select, would not lose the marker. I would say then that the visibility of that dataset should either turn off in both the image viewer and the spectrum viewer when the tool is inactive, or not for both (and leave it up to the user to uncheck one or both). |
There was a problem hiding this comment.
Not changing the plot scaling every time a new spaxel is selected helped a lot. Now there is a fixed reference frame for understanding differences in spaxel flux.
The spaxel spectrum plotting is a bit laggy and can cause confusion for the first spaxel selection. "Where is my spaxel spectrum?"
My remaining suggestions (primarily allowing two tools to be active) can be addressed in separate tickets.
| # Remove data from viewer, re-add data to app, and then add data | ||
| # back to spectrum viewer | ||
| self.viewer.jdaviz_app.remove_data_from_viewer("spectrum-viewer", label) | ||
| dc[label] = spec |
There was a problem hiding this comment.
is there any difference between this and calling app.add_data (looks like the only thing calling add_data would add is some label checks and a snackbar message)?
There was a problem hiding this comment.
The problem with that is you get a snackbar message every time you click on a new pixel.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
I share the same concerns the other devs have pointed out above, but also agree this is in a shippable state where we can begin testing with our users! Thanks for the feature @javerbukh! A few concerns:
- The icon is a little small in the plugin bar to the point it's hard to read. I assume @Jenneh designed it, in which case I'm willing to trust her here, but it is a little difficult for me to discern what the icon actually looks like
- Since we can click on multiple image windows, I was able to get into a state where I saw two spectrums for each spaxel I clicked in different windows. I couldn't find a way to discern the two of them since they're both magenta and we strip those options from the plot options. This seems like a big enough UX concern that we might want to consider it before we merge?
| # Remove data from viewer, re-add data to app, and then add data | ||
| # back to spectrum viewer | ||
| self.viewer.jdaviz_app.remove_data_from_viewer("spectrum-viewer", label) | ||
| dc[label] = spec | ||
| self.viewer.jdaviz_app.add_data_to_viewer("spectrum-viewer", | ||
| label, | ||
| clear_other_data=False) | ||
| self.created_data_labels.append(label) |
There was a problem hiding this comment.
I assume this section is to wipe the old spectrum and replace it with the new spectrum? It wasn't clear from the comment. Because there are other places in past where we've had to "toggle" the same thing on and off to get things to work, it might be worth saying that this is NOT one of those scenarios
There was a problem hiding this comment.
I don't think I understand. What would you like changed?
| def on_mouse_event(self, data): | ||
| event = data['event'] | ||
|
|
||
| if event == 'click': |
There was a problem hiding this comment.
I've never had to watch for mouse events before, but this feels heavy handed (this method is going to fire for all mouse events then check to see if it's a click event). Is there a way to monitor for ONLY click events? Trying to think about performance
There was a problem hiding this comment.
Yes, that is basically what it is doing under activate method. The if is still good to have in case we want to expand this logic to other mouse/key events.
self.viewer.add_event_callback(self.on_mouse_event,
events=['click'])There was a problem hiding this comment.
I'm not sure if there is another way to do this, do you have a suggestion @duytnguyendtn?
|
I tried to address all review comments that I could. If something was left out that you feel strongly about, please let me know. Otherwise this should be ready for final reviews. |
Co-authored-by: P. L. Lim <[email protected]>
There was a problem hiding this comment.
Should we add a note in the user doc that if you see spaxel spectrum as flat, try zoom in a lot because the signal/flux not summed like the original spectrum?
From the code, I expected the magenta stuff to auto-clear when I collapse the cube and only display the collapsed image in flux viewer, but it didn't. Did I misunderstand? Here is what I see.
|
|
||
| This tool allows a user to select one pixel in the image viewer and visualize the spectrum | ||
| at that point in the spectrum viewer. The selected pixel will be highlighted in the image viewer | ||
| and the spectrum will have the same color. If there are multiple data selected in the image viewer |
There was a problem hiding this comment.
I don't think it makes sense to even allow this on clicks outside of flux-viewer, since the spectrum-viewer only corresponds to flux-viewer. It gets too confusing to see spaxel from, say, uncert-viewer shown but we don't have a way to collapse anything outside of flux-viewer.
There was a problem hiding this comment.
I'll leave this up to the POs to decide as a follow-up ticket.
| event_x = data['domain']['x'] | ||
| event_y = data['domain']['y'] |
There was a problem hiding this comment.
If you ever wanna support multiple cubes in the same viewer (gasp), keep in mind that this is the X/Y of the reference data. FYI.
There was a problem hiding this comment.
Supporting multiple cubes will be it's own epic, not starting that work here 😉
Co-authored-by: P. L. Lim <[email protected]>
|
@pllim I'd prefer not to add that to the docs since the reason you need to zoom in so much is that the default collapse function for the spectrum viewer is sum, which causes some spectra (including spatial subsets a few pixels in size) to appear as a flat line at 0. Once we change that default function it should look as expected. As to your second point, the "X" stays until the tool is deactivated, and then it is removed. If the user still has the tool activated and proceeds to use other plugins, it will stay visible. |
Co-authored-by: P. L. Lim <[email protected]>
Description
This pull request adds a tool to cubeviz that allows a user to see the spectrum at a spaxel location on the image viewer. The tool is currently housed in the subset section. A spectrum is made for each active layer in the image viewer that is selected (as long as that layer is also a cube).
There are a few questions that need to be answered or things to be fixed, which can be found in the todo section.
Screen.Recording.2022-05-13.at.9.17.48.AM.mov
Todo
UserWarning: Given trait value dtype "float64" does not match required type "float64". A coerced copy has been created.app.add_data(spec, label)followed byapp.add_data_to_viewer("spectrum-viewer", label, clear_other_data=False)but the spectrum does not actually appear visible in the viewer. The current work around is to manually setfigure.marks[-1]to be visible and correctly scaled, but there should be a better way.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.
triviallabel.CHANGES.rst?