Add option to see full glue state tree for spectral subsets #2138
Conversation
javerbukh
requested review from
duytnguyendtn,
rosteen,
pllim,
kecnry,
bmorris3,
haticekaratay and
cshanahan1
as code owners
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #2138 +/- ##
==========================================
+ Coverage 91.93% 91.96% +0.02%
==========================================
Files 143 146 +3
Lines 15781 15939 +158
==========================================
+ Hits 14509 14659 +150
- Misses 1272 1280 +8
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
| @@ -828,7 +828,7 @@ def get_subsets_from_viewer(self, viewer_reference, data_label=None, subset_type | |||
| return regions | |||
|
|
|||
| def get_subsets(self, subset_name=None, spectral_only=False, | |||
| spatial_only=False, object_only=False): | |||
| spatial_only=False, object_only=False, simplify_spectral=True): | |||
There was a problem hiding this comment.
I think we'll eventually want this to default to False (or perhaps not have the kwarg at all and have a separate simplify_spectral_subsets() method which applies the change directly to the glue state). Is that the eventual goal or do others have different thoughts on the eventual direction?
This may still be the simplest incremental step towards that direction though and if so, is ok with me for now.
There was a problem hiding this comment.
@camipacifici mentioned that users would prefer the simplified version since it makes sense with what they're seeing visually. I can only see the non-simplified version being useful for Subset Plugin, although even there we may decide to stick with the simplified version. The reason being that there are multiple states (i.e. subset_state.state1, state1.state2, state1.state2.state1, etc.) that are duplicates and editing one may not reflect the changes in the viewer.
There was a problem hiding this comment.
ok, then simplify_spectral=True (as the default) could call the method first before exposing the information in the same dictionary format? I just think it would be nice to have a consistent output-format (between spectral and spatial subsets) for default kwargs. Right now, the default returned object is quite different between spectral and spatial 🤷 . I also worry if we are going to show the actual state in the plugin by default, but the simplified representation in the API, that that could cause some confusion for the user.
There was a problem hiding this comment.
I think in the event of confusion we should default to the simpler version. Is the main reason to keep the non-simplified version consistency with the spatial subset? They are different things so I think it makes sense to represent them differently.
There was a problem hiding this comment.
Right, but it is the same method and the output formatting changes based on the "context" (rather than user-inputs). In other words imviz.app.get_subsets() returns these dictionary structures (non-simplified), whereas specviz.app.get_subsets() returns a completely different type of object and which is now simplified and doesn't match what we'll eventually want to show in the plugin. I just think this is difficult for an API where users might want to parse the returned object, since they'll need to add custom logic to determine how to parse it.
But if this is staying within the app-level, we could also address this issue when exposing it to the helper or plugin API.
There was a problem hiding this comment.
My understanding is this is staying in the app-level and certain features can be exposed by helper methods.
There was a problem hiding this comment.
if its staying in app-level, then maybe the argument of what users would find most convenient doesn't apply yet 😉 . Ok, I won't hold up the PR for this and we can revisit later, but I am concerned that changing output formats will be a headache as we start to use this as well.
There was a problem hiding this comment.
I think I'm less concerned with changing the output format with this argument, than I am with the fact that the format within the same output dictionary is mixed in the Cubeviz case when you have both spatial and spectral subsets defined. The first solution that comes to mind is returning only the spectral subsets if simplify_spectral is True, but that would necessitate changing the default to False so that you always get everything back by default.
|
This surprised me, I think it's a bug but I'm not sure. In the screenshot below I used the
|
|
I think it only happens if you also have spatial subsets defined. Edit: And the spatial subset has to be defined first. Defining a spectral subset like this and then a spatial subset doesn't trigger it. |
|
I'm guessing that isn't directly related to this PR (can you check if it happens on main)? Maybe we can consider out-of-scope for the review here and file an issue/ticket? |
|
Ahh, right, this only implements the non-simplified output. The bug is also present on main, I'll open a separate ticket for that. My other comment applies to main as well then, I'll approve this. |
|
Any user facing stuff that warrants a change log? |
| all_subsets[label] = [reg['region'] for reg in subset_region] | ||
| else: | ||
| all_subsets[label] = subset_region | ||
| elif spatial_only and not is_spectral: |
There was a problem hiding this comment.
Why would something be spatial and spectral at the same time? Pondering for another day.
There was a problem hiding this comment.
is_spectral is a bool for whether the subset in question is spectral or not. So if the spatial_only flag is true and is_spectral is False then we add the subset information to the returned list.
Description
This pull request is to allow us to access the full glue state tree for creating a composite spectral subset. The way to access it is:
specviz.app.get_subsets(simplify_spectral=False)This PR also gives access to the subset state object in the dictionary that is returned by
get_subsets. That can be accessed using:specviz.app.get_subsets('Subset 1')[0]['subset_state']Fixes #2134
Change log entry
CHANGES.rst? If you want to avoid merge conflicts,list the proposed change log here for review and add to
CHANGES.rstbefore merge. If no, maintainershould add a
no-changelog-entry-neededlabel.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.