spectral extraction: improved logic for initial position of "Manual" background trace

[kecnry]

Description

This pull request improves the logic for the initial guess for the position of the "Manual" background trace. Previously it started at the same guess as the extraction trace (based on the brightest flux medianed over the dispersion direction). Now it uses the same location that would be adopted for a OneSided background, with the "direction" determined on whether the extraction trace is in the top or bottom half of the image, and the separation at 10% of the cross-dispersion "height".

Motivated by @ojustino's comment: #1682 (review)

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

[codecov]

Codecov Report

Base: 87.27% // Head: 87.65% // Increases project coverage by +0.37% 🎉

Coverage data is based on head (76b4e4b) compared to base (22559cf).
Patch coverage: 66.66% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1738      +/-   ##
==========================================
+ Coverage   87.27%   87.65%   +0.37%     
==========================================
  Files          95       95              
  Lines       10082    10131      +49     
==========================================
+ Hits         8799     8880      +81     
+ Misses       1283     1251      -32     

Impacted Files	Coverage Δ
...plugins/spectral_extraction/spectral_extraction.py	90.62% <66.66%> (+0.04%)	⬆️
jdaviz/configs/imviz/plugins/catalogs/catalogs.py	91.22% <0.00%> (-4.43%)	⬇️
jdaviz/app.py	93.76% <0.00%> (-0.27%)	⬇️
...z/configs/default/plugins/data_tools/data_tools.py	64.51% <0.00%> (+3.91%)	⬆️
jdaviz/configs/imviz/plugins/viewers.py	87.42% <0.00%> (+4.57%)	⬆️
jdaviz/configs/imviz/wcs_utils.py	88.34% <0.00%> (+20.38%)	⬆️
jdaviz/configs/imviz/plugins/compass/compass.py	93.33% <0.00%> (+23.56%)	⬆️

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.

[ojustino]

It looks like sign is different for the "Manual" and "OneSided" cases when brightest_pixel is in the top half of the image, but the same for both cases when brightest_pixel is in the bottom half. Since the example Specviz2D notebook's brightest pixel is in its image's top half, the initial guesses for the "OneSided" and "Manual" options end up on opposite sides of the brightest pixel.

I think the two guesses for "Manual" and "OneSided" should either always be in the same spot (like now, when brightest_pixel < width / 2) or always be opposite from another (like now, when brightest_pixel >= width / 2).

[kecnry]

Good point, I didn't explain the logic very well. The sign for the "OneSided" case is only applied if the separation would push the "TwoSided" case off the edge of the image, since the bg_separation traitlet is shared between "OneSided" and "TwoSided". In other words, the same logic that is used when "OneSided" becomes the default is used for the "Manual" position now, but bg_separation might still default to positive.

We could separate the bg_separation between the One and Two-Sided cases, but I personally don't think that is worth the overhead. The other option would be to have the "Manual" always match the position of the "OneSided" cases, if you think that would be more intuitive? (So always above the brightest pixel unless that would force it to go above the image, in which case it would be placed below instead).

[ojustino]

I agree that a simpler approach is better here and think having the "Manual" position match the "OneSided" position makes sense.

[kecnry]

these lines will be covered by #1739 (coverage was lost when skipping the remote test in #1736)

[ojustino]

The new commit sufficiently addresses my previous comment, and understood about the failing test. Should a merge here wait until the test has been rewritten? I imagine this branch would pass, but the change is also not an extremely urgent one.

[kecnry]

I think since we know this case works, it's ok to get in with the slightly lower coverage? This is the exact same case that was covered before, so these lines will be covered once that test is rewritten. But I'm also ok letting this PR sit as long as that test doesn't fall off the radar too long - I agree this PR isn't urgent and don't expect merge conflicts.

[pllim]

Code looks like code and we have a follow-up issue, so merging. Thanks!