Add option to get_data to apply spectral subset to collapse spatial

[javerbukh]

Description

This pull request is to add API to retrieve a data cube with a spatial subset applied to it, with a spectral subset then applied to that. The way to do that (using the CubevizExample notebook) is:

data_label = "jw02732-o004_t004_miri_ch1-shortmediumlong_s3d.fits[SCI]"
spatial_subset = "Subset 1"
spectral_subset = "Subset 2"
cubeviz.get_data(data_label, spatial_subset=spatial_subset, spectral_subset=spectral_subset, function=True)

There are also now get_data methods in Imviz and Specviz2D since the core get_data method no longer allows a subset to be applied.

Todo:

• Add tests
• Update docstrings
• Use in Line analysis
• Follow-up? Update documentation

Fixes #1354
Fixes #1490

Change log entry

• Is a change log needed? If yes, is it added to CHANGES.rst? If you want to avoid merge conflicts,
  list the proposed change log here for review and add to CHANGES.rst before merge. If no, maintainer
  should add a no-changelog-entry-needed label.

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 milestone set? Set this to bugfix milestone if this is a bug fix and needs to be released ASAP; otherwise, set this to the next major release milestone.
• After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

[codecov]

Codecov Report

Patch coverage: 90.90% and project coverage change: -0.16 ⚠️

Comparison is base (c4911b3) 91.78% compared to head (afca9a9) 91.63%.

❗ Current head afca9a9 differs from pull request most recent head 155bc85. Consider uploading reports for the commit 155bc85 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2199      +/-   ##
==========================================
- Coverage   91.78%   91.63%   -0.16%     
==========================================
  Files         147      148       +1     
  Lines       16262    16458     +196     
==========================================
+ Hits        14926    15081     +155     
- Misses       1336     1377      +41     

Impacted Files	Coverage Δ
...igs/specviz/plugins/line_analysis/line_analysis.py	97.76% <ø> (ø)
jdaviz/core/helpers.py	87.84% <81.81%> (-0.70%)	⬇️
jdaviz/configs/cubeviz/helper.py	97.80% <87.50%> (-1.05%)	⬇️
jdaviz/configs/specviz/helper.py	76.92% <90.90%> (+0.77%)	⬆️
...nfigs/cubeviz/plugins/tests/test_cubeviz_helper.py	100.00% <100.00%> (ø)
jdaviz/configs/imviz/helper.py	97.14% <100.00%> (+0.03%)	⬆️
jdaviz/configs/specviz2d/helper.py	86.27% <100.00%> (+0.27%)	⬆️
jdaviz/core/template_mixin.py	93.23% <100.00%> (-0.53%)	⬇️

... and 12 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[camipacifici]

It does what it is supposed to do, IF function=True.
If I use function='median', it does not crash, but it does not apply the spectral subset either.

When applying the spectral subset, it returns a cropped spectrum, which is an ok behavior, but it is different than what get_data does in specviz, i.e. return a masked spectrum. The two should return the same thing, conceptually.

spectral_to_spatial does not sound super intuitive to me. spectral_subset could be an alternative maybe? In this light, I tried to extract a spectral region from the collapsed spectrum from the full cube doing
regiononfullcube = cubeviz.get_data('my_data',
function=True,
spectral_to_spatial='Subset 2')
but it returned the full spectrum from the full cube, not just the spectral subset portion. No traceback. In this context _to_spatial would not make a ton of sense.

[javerbukh]

My issue with spectral_subset is a user could confuse that with subset_to_apply when working in specviz since those two sound like the same thing. What about apply_spectral_to_collapse?

Looks like my logic did not cover that case! I thought we only get the collapsed data when applying a spatial subset but I forgot that function can do that as well. I'll work on all of these points, thank you @camipacifici !

[kecnry]

My issue with spectral_subset is a user could confuse that with subset_to_apply when working in specviz since those two sound like the same thing

Is that still an issue though if this is only exposed in the cubeviz helper?

[camipacifici]

From a user perspective, I would probably expect to be able to call subset_to_apply with the same output in both cubeviz and specviz. As of now, subset_to_apply applies a spatial subset to a cube in cubeviz and a spectral subset to a spectrum in specviz.
This is a little late in the game, but would it be an option to leave subset_to_apply for spectral subsets and change the name of the keyword for the spatial subset collapse? Sorry, I did not catch this use case earlier.

[javerbukh]

I'm not sure about that workflow. If we want to separate the parameters, we could do spectral_subset and spatial_subset. I could also see moving cls to after the subset calls. Maybe something like:

cubeviz.get_data(data_label=None, spatial_subset=None, spectral_subset=None, cls=None, function=None)
and
specviz.get_data(data_label=None, spectral_subset=None, cls=None)

So that a user can do cubeviz.get_data('name', 'Subset 1', 'Subset 2', function=True) and specviz.get_data('name', 'Subset 1')

How does that sound @camipacifici and @kecnry ?

[camipacifici]

What is cls again?
I like your proposal. I think @rosteen coded up the subset_to_apply part and might have opinions here.

[pllim]

cls tells it what class you want your data returned as (e.g., Spectrum1D). If not given, it uses software default for the particular config.

[kecnry]

I like that plan, as long as somewhere handles the logic of checking to make sure spatial_subset and spectral_subset get subsets of the correct type and raise useful error messages otherwise (especially if passed as positional arguments). It also is probably a good idea for us to always pass these as keyword arguments in official examples/notebooks/etc to avoid that possible confusion.

I briefly thought to propose switching the order since spectral subset might be the more common use-case to request, but I like the way you propose as more intuitive to the order things are applied.

[kecnry]

If spatial_subset and/or spectral_subset is provided but function=None, cls=None, do we return a masked cube or assume function=True behavior?

[javerbukh]

My thought was if both spectral and spatial subsets have subset names, then we assume the user wants the spectral subset applied to the collapsed data. In that case, function will just be True. If a user also uses a function such as "median", then we will use that instead of the default specviz viewer function value.

[javerbukh]

When applying the spectral subset, it returns a cropped spectrum, which is an ok behavior, but it is different than what get_data does in specviz, i.e. return a masked spectrum. The two should return the same thing, conceptually.

Trying to get this working but not sure if we can use the same data_translator.get_handler_for(Spectrum1D).to_object(subset) to apply a spectral subset to a collapsed spatial subset. Maybe this correlates to the data names in self.app.data_collection.subset_groups[0].subsets? That is something I am still investigating.

Edit: Found a work around, latest commit implements it. Now working on updating docstrings and making tests. @camipacifici please let me know if the current implementation works better for you!

[kecnry]

That cannot happen if function=False, so the code ignores function in that case.

Should it raise an error instead?

[javerbukh]

Turns out that model fitting uses mask = False to show where a spectral subset is being applied. I changed line 520 in helpers.py to reflect that. Please examine the returned object for get_data to ensure that mask is what you would expect it to be. Hopefully I have covered all edge cases here but if another is found please let me know!

[kecnry]

All the configs act as I'd expect now, except for mosviz, but that should be as simple as copying the logic from specviz2d.

This also does prevent downstream apps from using this for non-spectral/spatial subsets. I'm working on testing against lcviz to propose some changes that would still allow for that flexibility while retaining the nice checks you've added here.

[kecnry]

See javerbukh#15 for proposed changes to support downstream implementations (will still require minor modifications to lcviz).

[javerbukh]

@kecnry Is there a reason that needs to be added to this PR and cannot be a follow-up? This PR's scope is already wider than initially planned.

[kecnry]

as-is this does break existing behavior downstream, but as long as we're willing to consider the follow-up PR a blocker for the release, I can do that as a separate PR after this is merged if you'd prefer.

[javerbukh]

I'm just weary of adding more complexity and changes to this PR when I've spent a couple days now fixing bugs that crop up each time something changes. I would be more comfortable for that PR to exist in jdaviz where other devs can review and give their thoughts.

[kecnry]

This looks good as long as we address the downstream concern as an immediate follow-up PR (I can open once this is merged) and before releasing 3.5.

[cshanahan1]

i can't find any issues with this, so ill give it a good ol' 'LGTM'