re-usable dataset select component #1221
Conversation
kecnry
force-pushed
the
data-select-component
branch
5 times, most recently
from
492b218 to
d959d72
Compare
Codecov Report
@@ Coverage Diff @@
## main #1221 +/- ##
==========================================
- Coverage 78.00% 77.96% -0.04%
==========================================
Files 90 90
Lines 7187 7179 -8
==========================================
- Hits 5606 5597 -9
- Misses 1581 1582 +1
Continue to review full report at Codecov.
|
| v-model="n_moment" | ||
| v-model.number="n_moment" |
There was a problem hiding this comment.
fixed this while I was here... using .number along with IntHandleEmpty removes the need for error handling in the plugin.
| # retrieve the data from the cube, not the collapsed 1d spectrum | ||
| self.dataset._viewer_refs = ['flux-viewer', 'spectrum-viewer'] |
There was a problem hiding this comment.
this feels a little hacky - is it worth generalizing more or exposing a more friendly method to do this?
| if ndec > 0: | ||
| if ndec > 0 and not np.isinf(ndec): |
There was a problem hiding this comment.
ran into a traceback when testing and clearing all data from the viewer (range goes to zero, ndec goes to infinity)
| assert mv.dc_items == ['has_simple_meta[DATA]', 'has_nested_meta[DATA]', 'no_meta'] | ||
| assert mv.dataset.labels == ['has_simple_meta[DATA]', 'has_nested_meta[DATA]'] |
There was a problem hiding this comment.
entries without metadata are filtered out by the DatasetSelect component now
There was a problem hiding this comment.
Will this confuse the users when they don't see it under selection even though they loaded it?
There was a problem hiding this comment.
Also, this list does not update when metadata is added later (see snippet). I propose we don't try to be too smart here, just list everything and user sees nothing if there is no metadata, as before.
import numpy as np
from jdaviz import Imviz
a = np.random.random((10, 10))
imviz = Imviz()
imviz.load_data(a, data_label='test1')
imviz.app
imviz.app.data_collection[0].meta['1'] = 1 # Dropdown still shows nothingThere was a problem hiding this comment.
I don't think it would be confusing, but we can ask Jenn. All of the dropdowns now filter to relevant entries, so I see this as consistent with that (it doesn't make much sense to be able to fit a model to a model, etc). But if we want to show "no metadata for ..." and still include them in the dropdown, I can make that happen.
If updating metadata manually is something we want to support, I can try to support that... but it also was not fully supported before this PR. Although the entry may have always shown in the dropdown, the displayed metadata would not update if you changed it manually after load, unless possibly if changing the selection.
There was a problem hiding this comment.
The problem with the notebook workflow is that we cannot predict what user wanna do with this thing. Say, they load data, and then they do some funky processing with it and decide to attach history of said processing as metadata, who are we to deny them?
Should updating metadata trigger display update? Maybe and can be future PR if requested.
Should plugin just not allow showing metadata at all if it is not there on load? I think that is a step backward.
There was a problem hiding this comment.
Do we consider accessing and modifying app.data_collection as user-facing? I suspect changing items in the data collection directly will cause issues across the app as we don't listen to changes from there anywhere (aperture photometry, for example, pulls values from metadata when the dropdown changes but won't update in real-time either, etc). I'm just worried this is a slippery slope. But if we do want to support user changes to things within data collection, then I would prefer disabling that ability entirely until a full solution is implemented rather than relying on the user to realize the distinction between live-updates and having to toggle a selection to pull the latest values.
I think the decision of whether items without metadata should be excluded or show "no metadata" is independent though, and I don't have a terribly strong opinion except that I always personally prefer self-consistency and showing a message is actually additional logic in the plugin.
| self.results_centroid = temp_result.to_value(u.AA) | ||
| self.results_centroid = temp_result.to_value(u.AA, equivalencies=u.spectral()) |
There was a problem hiding this comment.
ran into a bug when testing when the spectrum was in frequencies
There was a problem hiding this comment.
Is this bug out of scope here? If so, is there a follow-up issue?
| assert len(specviz_viewer.figure.marks) == 1 | ||
| assert len([m for m in specviz_viewer.figure.marks if isinstance(m, LineUncertainties)]) == 0 | ||
|
|
||
| specviz_viewer.show_uncertainties() | ||
|
|
||
| assert len(specviz_viewer.figure.marks) == 2 | ||
| assert len([m for m in specviz_viewer.figure.marks if isinstance(m, LineUncertainties)]) == 1 | ||
|
|
||
| specviz_viewer.clean() | ||
|
|
||
| assert len(specviz_viewer.figure.marks) == 1 | ||
| assert len([m for m in specviz_viewer.figure.marks if isinstance(m, LineUncertainties)]) == 0 |
There was a problem hiding this comment.
changed this since the LineAnalysis plugin might add (hidden) marks, so let's just check against the ones we expect for uncertainties.
jdaviz/core/template_mixin.py
Outdated
| @cached_property | ||
| def selected_obj(self): | ||
| if self.selected == self._default_text: | ||
| if self.selected == self._default_text or self.selected not in self.labels: |
There was a problem hiding this comment.
this is needed since this could be triggered by an observer in a plugin WHILE an error is raised on an invalid selection and before the default can be applied
kecnry
force-pushed
the
data-select-component
branch
from
d959d72 to
023d846
Compare
kecnry
requested review from
duytnguyendtn,
rosteen,
javerbukh,
ojustino and
pllim
as code owners
There was a problem hiding this comment.
In the Moment Map plugin, I dunno how I feel about "Spectral Region" dropdown not appearing until there is a spectral subset created. At first I thought it was a bug and got confused. Also, I use that dropdown to remind me that I can actually create moment map with Subset. Without that dropdown to remind me, I won't get reminded until I create a Subset, which I might not. Maybe that's just me? 🤷
I did this in a number of places, but it is easily changed by setting |
|
+1 from me for keeping the subset dropdowns visible even if there are currently none as a reminder that subsets could be applied to the operations in the plugin. |
There was a problem hiding this comment.
Looks pretty good, other than agreeing with @pllim's comment as I said already. The filter logic made enough sense for me to see how it's used, but I'm definitely adding that to my mental list of "things that should be explained in the developer docs." I'm going to hold off on approving until the subset selection default behavior is updated.
| </v-row> | ||
| </div> | ||
| </template> | ||
| <script> |
There was a problem hiding this comment.
Looking through this, I realized I don't have a good idea of what module.exports does on top of the other vuetify stuff, despite it being used in a bunch of the .vue files. I should go do some reading 😅
There was a problem hiding this comment.
You can do a lot in the exports, but here the props are the properties/arguments that are accepted when calling the template component from another template.
|
I changed it so that all subset dropdowns are always visible (and dataset dropdowns are always visible in mosviz, since I think its important to give some indication of the change as you change rows). |
|
"Plot all" of SDSS lines on Cubeviz example notebook seems very slow but I can't tell if it is related to this refactoring or not. |
kecnry
force-pushed
the
data-select-component
branch
from
fe6831c to
a30cbfa
Compare
|
Going off of @pllim's review, I do see another weird interaction.
There should be nothing showing in the drop down for layer options and a traceback in the console. |
|
Hmm weird that Metadata Viewer from Imviz would call this:
|
TODO: * implement spatial subset support for model fitting * test/finish gaussian smooth in cubeviz * cleanup model fitting
* to make it clear that the plugin does take subsets as an option and give a hint to the user to create them
calls to get_data/subset still require passing ref, so both are stored internally
kecnry
force-pushed
the
data-select-component
branch
from
a30cbfa to
1a56f01
Compare
* although calling this will still likely raise an error... bottom line is that we can't call get_object or selected_obj in imviz for viewers without ids.
* otherwise calling for imviz viewers without assigned references will fail without passing a known object class (which we cannot always assume to be Spectrum1D)
* and show message saying no metadata is available * see spacetelescope#1221 (comment)
|
Found one more thing: Layer does not show anything when
|
|
Thanks! |
Description
This pull request implements a re-usable dataset select component with:
This also creates a new
BaseSelectPluginComponentwhich holds shared logic between theDatasetSelect,ViewerSelect(#1195), andSubsetSelect(#1156, #1175).Finally, this splits the spatial subsets into their own dropdown in model fitting. For cubeviz this will effectively replace the data dropdown since the data dropdown is hidden as it is only populated with a single choice (unless more data is manually added).
Below are some before (left) & after (right) screenshots:
Fixes #1220
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?