Subset refactor part 1: new get_data API

[javerbukh]

Description

This pull request is to address one part of #1921, namely it introduces a new API that changes how data retrieval works.

I will remove the concept notebook when we (probably I again) have transferred the thoughts and notes from there to somewhere more permanent.
EDIT: Original issue has been updated.

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

Base: 92.04% // Head: 92.06% // Increases project coverage by +0.02% 🎉

Coverage data is based on head (42faf39) compared to base (892ff36).
Patch coverage: 96.29% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1984      +/-   ##
==========================================
+ Coverage   92.04%   92.06%   +0.02%     
==========================================
  Files         140      140              
  Lines       15206    15285      +79     
==========================================
+ Hits        13996    14072      +76     
- Misses       1210     1213       +3     

Impacted Files	Coverage Δ
jdaviz/core/helpers.py	73.87% <94.00%> (+3.87%)	⬆️
...nfigs/cubeviz/plugins/tests/test_cubeviz_helper.py	100.00% <100.00%> (ø)
jdaviz/configs/imviz/tests/test_helper.py	100.00% <100.00%> (ø)
jdaviz/configs/imviz/tests/test_regions.py	100.00% <100.00%> (ø)
jdaviz/configs/specviz2d/tests/test_helper.py	100.00% <100.00%> (ø)

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 at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[kecnry]

I am liking the simplicity of the API and the lack of a returned dictionary more and more. I haven't actually tried this out yet, but how is it expected to work for the flux cube in cubeviz (how do you tell the API whether to access the cube or collapsed spectrum with the same label)?

[kecnry]

I don't love this being in the method for all helpers since its only applicable to cubeviz... but also see how it would be tricky to overload this for the cubeviz helper. Another option might be to_object_kwargs={} and then overload in cubeviz to have an explicit statistic kwarg.

[javerbukh]

I use the statistic parameter in a few places in the code so I am hesitant to take it out and only use it for cubeviz.

[rosteen]

how is it expected to work for the flux cube in cubeviz (how do you tell the API whether to access the cube or collapsed spectrum with the same label)?

Correct me if I'm wrong @javerbukh, but it looks like you do this by providing the statistic you want to collapse with. So you're not directly getting out what's already collapsed in the spectrum viewer, you're doing the collapse again.

Jesse, I just started testing this, and it looks like the min and max options are broken for statistic. I was testing that with a spatial subset, and those two statistic options return the original (I think) Glue Data object rather than a collapsed Spectrum1D.

[javerbukh]

@rosteen That is correct.

Ok thank you for letting me know! I will check that out.

[kecnry]

it looks like you do this by providing the statistic you want to collapse with. So you're not directly getting out what's already collapsed in the spectrum viewer, you're doing the collapse again.

Ok, that makes sense. Will that provide any headaches when migrating internal calls (we'll need to pull the statistic from the spectrum viewer's plot options or state and pass it through the get_data method)?

[javerbukh]

@kecnry I think what you said is correct, if we want to get the collapse spectrum in cubeviz we will need to call get_data(data_label, statistic=cubeviz.app.default_spectrum_viewer).

[kecnry]

I think its a bit uglier actually: get_data(data_label, statistic=cubeviz.app.get_viewer(cubeviz._default_spectrum_viewer_reference_name).state.function), but we could probably write a cubeviz.collapse_function property so that it will then appear as get_data(data_label, statistic=cubeviz.collapse_function)?

[javerbukh]

Talking to @rosteen we realized that the issue was he was using statistic='min' instead of statistic='minimum'. The problem also is that something is returned that way that is not the minimum spectrum. The solution to that will be running statistic through a list of valid values before passing it to glue-astronomy.

@kecnry I'm in favor of writing a collapse_function property, but I would direct that work to #1988 .

[rosteen]

I noticed one more thing while testing: retrieving data from Imviz with a spatial subset applied appears to hit a bug and returns the Glue data instead of an NDData array:

[javerbukh]

@rosteen Should be fixed now.

[rosteen]

Working in all the cases I've tested now. I still wish you could specify multiple subsets (e.g., a spatial and a spectral in Cubeviz) but we can leave that for potential future enhancements. Approved.

[kecnry]

Should providing statistic when the underlying data is not a cube raise an error (its currently silently ignored which could result in unnoticed typos)?

[javerbukh]

@kecnry can you provide a test case? The only situation where a non-cube will have get_object call with statistic present is in the case of a spectrum1d object with a shape of 1. In that case, what is the difference if statistic is called on it? You mentioned there being a typo but there is already a check for only valid statistic values at the top of the code block.

[kecnry]

can you provide a test case? The only situation where a non-cube will have get_object call with statistic present is in the case of a spectrum1d object with a shape of 1.

get_data is implemented at the core helper level, which means it's technically accepted as an argument in all configs. If the user passes statistic in Imviz or Specviz, should that raise an error or be silently ignored?

You mentioned there being a typo but there is already a check for only valid statistic values at the top of the code block.

I can't remember if I reviewed before that was added or just missed it - but that looks sufficient for the typo concern - thanks!

[javerbukh]

@kecnry Ok, let me know if the latest change satisfies the criteria.

[kecnry]

let me know if the latest change satisfies the criteria.

Looks good to me - thanks!

Just needs a change log (and CI green) before merging.

[pllim]

Imviz still works. Belated 👍 from me. Thanks!