add data to viewers when created by plugins #974
Conversation
kecnry
force-pushed
the
plugins-show-results
branch
2 times, most recently
from
1250388 to
a9371dc
Compare
Codecov Report
@@ Coverage Diff @@
## main #974 +/- ##
==========================================
+ Coverage 70.46% 70.50% +0.04%
==========================================
Files 73 74 +1
Lines 5393 5472 +79
==========================================
+ Hits 3800 3858 +58
- Misses 1593 1614 +21
Continue to review full report at Codecov.
|
kecnry
requested review from
duytnguyendtn,
ibusko,
javerbukh,
ojustino,
pllim and
rosteen
as code owners
There was a problem hiding this comment.
Seems to work as advertised, except I cannot figure out how Gaussian Smooth is supposed to behave in Mosviz. I did something and result didn't show, so I must have used it wrong, a second dev to test out Mosviz would be nice. I tested the rest in Lab and they worked fine.
Thanks!
| # selected_viewer for selected_axis == 0 | ||
| # NOTE: this is currently cubeviz-specific so will need to be updated | ||
| # to be config-specific if using within other viewer configurations. | ||
| viewer_to_id = {'Left': 'cubeviz-0', 'Center': 'cubeviz-1', 'Right': 'cubeviz-2'} |
There was a problem hiding this comment.
Seems weird to have Cubeviz specific setting in a "default" section of plugins but seems to work with the other viz, so probably okay for now.
There was a problem hiding this comment.
I agree, but couldn't think of a (simple) general way to reference the image viewers by position (suggestions welcomed). As far as I can tell, collapse is only used in cubeviz, so maybe we should just move the plugin there instead of in default?
There was a problem hiding this comment.
I don't see why anyone wants to collapse anything besides a cube, so I agree it should be moved but maybe as a separate PR.
There was a problem hiding this comment.
Honestly I think there are a few things in the package that would make better sense if they were located elsewhere. I agree that there should be a separate PR to do a little reorganizing.
This comment has been minimized.
This comment has been minimized.
kecnry
force-pushed
the
plugins-show-results
branch
from
abed1d2 to
11b7716
Compare
This comment has been minimized.
This comment has been minimized.
|
I fixed RTD on |
* each plugin has a switch and/or select box which shows where data will appear and give options to change the default * spectrum viewer implements a basic color cycler for new entries
* tests fail to update viewer since there is no viewer in the passed app, so instead we'll just set the boolean switch to false to allow the rest of the tests to run
kecnry
force-pushed
the
plugins-show-results
branch
from
11b7716 to
dc0af85
Compare
There was a problem hiding this comment.
I was able to see the data get added to the viewers. However, it's causing some very strange behavior. In Specviz, running one of plugins with the auto-plot toggled on causes my subset to iterate to a new color, but only in the plot itself. If I use another plugin, the plot color toggles again but now all the GUI labels catch up to the previous color. Super weird, and I don't see anything like in on main when using the Model Fitting plugin, which currently auto-plots the results. See screenshots below. In Cubeviz using the smoothing plugin I see the smoothed cube show up in the specified viewer, but I can't change any of the layer options (e.g. switching to Square Root scaling).
| # https://sashamaps.net/docs/resources/20-colors/ | ||
| color_cycle = ["#595959", "#e6194B", "#3cb44b", "#4363d8", "#f58231", | ||
| "#911eb4", "#42d4f4", "#f032e6", "#bfef45"] | ||
| self.state.layers[-1].color = color_cycle[(len(self.state.layers) - 1) % len(color_cycle)] |
There was a problem hiding this comment.
This seems to be interacting with subsets incorrectly, see screenshots on the main thread.
There was a problem hiding this comment.
@rosteen - good catch. I reworked this logic in 1c0986f to only use the color cycler for data layers (ignoring subset layers) and I think it now gives the expected behavior, but please confirm. I think some day merging the color cycler between subsets and data layers might be a good idea, but think that's probably out of scope for now.
|
Video: Screen.Recording.2021-12-06.at.12.22.18.PM.mov |
* some day it might be nice to use a SHARED color cycler between the subsets and the data layers, but for now this avoids some odd behavior where subset layers were changing colors when adding new data layers
kecnry
force-pushed
the
plugins-show-results
branch
from
9619b9b to
1c0986f
Compare
|
That fixed the subsets changing color, but I see multiple other problems with the color cycling. I'll write them up in a minute when I can summarize what's going on, but in the meantime at this point I would recommend removing the color cycling from this PR and making that a separate effort. It's not necessary for the automatic data addition as far as I know, just a nice to have. |
|
Ok, here's the color cycling problems I'm seeing:
|
|
@kecnry I think this got missed, from upstream:
Do you see the same/any idea what might be causing this? I have to de-select and re-select the data to get any layer changes to stick (they reset on reselection, but afterwards I can actually change them). It's possible this is another existing bug (bad |
I vote that we ask for opinions on that at tagup tomorrow morning, I could go either way. |
Were opinions given at yesterday's tag-up? What is the path forward? |
Yes, we decided to move forward with this PR as is and fix the bug later (although this PR likely raises its priority as it is more prevalent now). @rosteen - just want to double check if all of your comments have now been addressed (or deferred 😉) and we can merge? |
Description
This pull request provides immediate feedback from all plugins by showing (or replacing) data in the viewers. In cases where data would be replaced in an image viewer, these currently default to NOT replacing, but still provide the option to replace the image in any of the available viewers and text
(should be reviewed by @Jenneh)to explain that the new data will be available in the data menu.The following plugins are affected:
This also implements a color cycler for the spectrum viewer - so that new data entries default to have a new color rather than always dark gray. This makes it easier to see the presence of new data in the viewer if its similar to existing data (especially in the case of gaussian smoothing with a small width, for example).(color cycler split into #988 and deferred)The following screenrecordings show a general overview of most of the cases:
Screen.Recording.2021-12-03.at.10.03.56.AM.mov
Screen.Recording.2021-12-03.at.10.02.20.AM.mov
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?