re-usable spectral subset dropdown component with color indicator

[kecnry]

Description

This pull request implements a color indicator in all plugin spectral subset dropdowns. In order to do so, it creates a new SpectralSubsetSelectMxn which applies the logic on the python side to the plugins, as well as a plugin-subset-select vue component. If needing customization outside the defaults of the mixin (as is the case with the line analysis plugin which needs two separate subset select instances), we can create traitlets manually and attach to the plugin with self.continuum = SpectralSubsetSelect(self, 'spectral_subset_items', 'spectral_subset_selected'). This allows us to isolate all logic for subset selection to a single place for self-consistent across plugins and minimizing code duplication. In the future, we should consider doing this for all re-used components/code in plugins as we edit them.

Screen.Recording.2022-03-09.at.3.18.16.PM.mov

This also removes the spectral bounds from the moment map and collapse plugins. In its place is a warning that will show when the selected subset contains subregions that the plugin will take the full range of the region (at least until we update that behavior).

Screen.Recording.2022-03-14.at.10.18.00.AM.mov

🐱

TODO:

• filter out spatial subsets - this was done in some of the plugins (not all) but the same solution does not work here because the subsets themselves don't exist at the point they're being added to the dropdown list.
• implement for line analysis' second spectral subset selection (with different traitlets...) for continuum region.
• finalize icon decision - icon used in the glue-subset menu is not in mdi, but we could probably still pull it in. Since they're only spectral, I thought a more specific icon that suggests that might be useful.

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.

• Are two approvals required? Branch protection rule does not check for the second approval. If a second approval is not necessary, please apply the trivial label.
• Do the proposed changes actually accomplish desired goals? Also manually run the affected example notebooks, if necessary.
• Do the proposed changes follow the STScI Style Guides?
• Are tests added/updated as required? If so, do they follow the STScI Style Guides?
• Are docs added/updated as required? If so, do they follow the STScI Style Guides?
• Did the CI pass? If not, are the failures related?
• Is a change log needed? If yes, is it added to CHANGES.rst?
• Is a milestone set? Milestone is only currently required for PRs related to Imviz MVP.
• After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

[kecnry]

might be able to turn these into cached properties once we create another component for the data entry and have them talk to each other (so that the underlying data entry can be cached and accessed directly instead of needing to pass it to the method).

[pllim]

Will caching here really save anything? By the time this gets called, the whole cube is there. I think it is better to catch where you can prevent the cube from being recreated in the first place (most logically using its data label) but then you face the problem of cache invalidation. 🤷

Also, do selected_min and selected_max need to be two different methods? Will there be a case where you only need one but not the other? Is it better to have a selected_minmax or selected_cube_limits (or whatever you want to call it) that returns both at once?

[kecnry]

In this case, it probably wouldn't be a significant savings to cache, unless the cube is really large. But ease-of-caching is one thing we gain by this refactor, so I think we should use it when possible to keep ahead of optimization efforts.

I'm guessing this will get removed entirely if/when we support subsets with subregions in collapse/moment maps. Since those are the only two cases that currently use it, I have no objection to switching to a single method.

[pllim]

Do you think this is more of a "new feature"?

[kecnry]

I don't have a strong feeling either way, it just feels like too small of a change (for the user) to qualify as a "feature", but have no problem moving it if you think it belongs there?

[pllim]

"New feature" is more visible to the user if you think users are going to be super excited about this. I'll leave it to your good judgement.

[pllim]

Maybe we should remove :no-inheritance-diagram: from .. automodapi:: jdaviz.core.template_mixin because at this point, the diagram might be useful.

Is it enough for future development work to only have docstrings? Do we need more docs under https://jdaviz.readthedocs.io/en/latest/dev/index.html ? For instance, if we decide to extend this to Imviz or non-spectral subset, will someone be able to figure it out from existing doc?

I would also like Brian C. to have a look as well, in case this somehow affects MAST.

Maybe squash the commits before final review, unless you want commits like "trying to make RTD happy" to be in the history. Or we can use that "Squash and Merge" button. Up to you. 😉

[pllim]

Suggested change

	This Base class handles attaching traitlets from the plugin itself to logic
	This base class handles attaching traitlets from the plugin itself to logic

[pllim]

If this is eventually going to be the base class for all plugin components, it needs to be better documented.

I almost want to suggest abstract/meta class but maybe that is overengineering.

[kecnry]

Thanks for the review/suggestions!

Maybe we should remove :no-inheritance-diagram: from .. automodapi:: jdaviz.core.template_mixin because at this point, the diagram might be useful.

Probably would be useful for devs, but unlikely for users - should I include that in this PR or should we handle that separately?

Is it enough for future development work to only have docstrings? Do we need more docs under https://jdaviz.readthedocs.io/en/latest/dev/index.html ? For instance, if we decide to extend this to Imviz or non-spectral subset, will someone be able to figure it out from existing doc?

Good points, I'm not sure. I think using the existing component is pretty straightforward and covered by the docstrings and existing examples. But writing other components might be a little more difficult... I just have no idea the best place/way to document that.

I would also like Brian C. to have a look as well, in case this somehow affects MAST.

I see no reason that this should affect MAST, its just internal code refactoring and shouldn't touch any embedding/css, but always welcome extra reviews!

Maybe squash the commits before final review, unless you want commits like "trying to make RTD happy" to be in the history. Or we can use that "Squash and Merge" button. Up to you. 😉

I always prefer the "squash and merge" so the individual commit history can live in the PR, but since someone else might beat me to it without squashing, I'll see if I can clean up the history to just a few commits.

[codecov]

Codecov Report

Merging #1156 (20bc97a) into main (394dea0) will increase coverage by 0.19%.
The diff coverage is 90.62%.

@@            Coverage Diff             @@
##             main    #1156      +/-   ##
==========================================
+ Coverage   76.17%   76.36%   +0.19%     
==========================================
  Files          87       87              
  Lines        6685     6716      +31     
==========================================
+ Hits         5092     5129      +37     
+ Misses       1593     1587       -6     

Impacted Files	Coverage Δ
...igs/default/plugins/model_fitting/model_fitting.py	31.05% <85.71%> (-0.65%)	⬇️
...igs/specviz/plugins/line_analysis/line_analysis.py	78.80% <86.66%> (-5.97%)	⬇️
jdaviz/core/template_mixin.py	93.18% <90.52%> (-6.82%)	⬇️
jdaviz/app.py	90.29% <100.00%> (+0.01%)	⬆️
...configs/cubeviz/plugins/moment_maps/moment_maps.py	94.11% <100.00%> (+4.02%)	⬆️
...daviz/configs/default/plugins/collapse/collapse.py	97.05% <100.00%> (+11.18%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f40a7c7...20bc97a. Read the comment docs.

[pllim]

Let's remove :no-inheritance-diagram: here. It is a one-liner.

[pllim]

Overall, this works as advertised. My comments about dev docs or combining methods are suggestions and do not block merge, so I am approving. Not sure about your comment about finalizing icon, is that a Jenn question?

Thanks. Great work!

[javerbukh]

I like the idea of using the mixins to remove some duplicated code, and I think the UI looks great! My only question would be is there anyway to cover more lines by tests? Otherwise, this is good to go! I'll leave it un-merged until after the tag-up in case anyone wants to take a look at it before it goes in.

[kecnry]

Thanks @javerbukh - I'd like to address as many of @pllim's remaining documentation suggestions that I can first and then squash/merge if no one has other comments/suggestions by then (I also reached out to Jenn about the icon, but that can always be iterated on down the road as well).

[Jenneh]

Sounds good to me.