spectral-extraction: improved initial parameter estimation

[kecnry]

Description

This pull request improves the automatic initial input parameter determination for input to spectral extraction, particularly for cases where the previous defaults would result in a window outside the range of the image which would throw an error from specreduce. This also prevents other cases like this from throwing a traceback by instead raising an error snackbar message on load and pointing to the plugin for manual extraction.

Re failing coverage checks: I did add test coverage for the near-edge example to ensure it forces a one-sided background, but since the parser only accepts files, its difficult to add coverage for the "brightest pixel on edge" logic and the complete try/except snackbar.

TODO:

• iterate on expected behavior
• add test coverage

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?
• After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

[codecov]

Codecov Report

Merging #1560 (ff60cd5) into main (6060929) will decrease coverage by 0.02%.
The diff coverage is 78.94%.

@@            Coverage Diff             @@
##             main    #1560      +/-   ##
==========================================
- Coverage   85.64%   85.61%   -0.03%     
==========================================
  Files          94       94              
  Lines        9216     9228      +12     
==========================================
+ Hits         7893     7901       +8     
- Misses       1323     1327       +4     

Impacted Files	Coverage Δ
jdaviz/configs/specviz2d/helper.py	80.00% <60.00%> (-1.93%)	⬇️
...plugins/spectral_extraction/spectral_extraction.py	88.60% <85.71%> (-0.33%)	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[camipacifici]

Looks good to me!

[pllim]

Is there a follow-up issue to make Specviz2D parser more flexible and then add the edge case you couldn't add before in this PR?

[pllim]

What does the removal of the second condition do?

[kecnry]

previously this avoided tracebacks before the original 1d spectrum was extracted. Now this will force that error to occur, which is now caught by the try/except and exposed in the snackbar.

[pllim]

What is < 0 testing? You sure you don't want to be more explicit with numpy.testing.assert_allclose()?

[kecnry]

It's really just testing that the logic detected that the extraction window was so close to the top of the image, that the background region needed to be below the extraction region. I was intentionally non-specific in the test, because we'll very likely improve the separation/width default logic in the future which would change the exact value, but as long as the window-detection logic doesn't change significantly to avoid the edge entirely, that should still always return a negative separation.

[kecnry]

Is there a follow-up issue to make Specviz2D parser more flexible

Yes there is, I'll add a note in there to also extend the test cases 🐱

[pllim]

LGTM. Thanks!