Fix 2D smooth visibility bug by utilizing SpectralExtraction linking logic

[duytnguyendtn]

Description

Big thanks to Jesse and Kyle for the hack session that found this solution! This PR fixes #1956 by borrowing the linking logic from the Spectral Extraction plugin for any non-cube linking scenarios.

Turns out the fix is slightly more complicated than the one we figured out together; the linking logic for Spectral Extraction doesn't work for smoothed data being linked against cubes (mainly in the case of Cubeviz). Because Cubeviz wasn't impacted by this bug to begin with, and the observation that the Spectral Extraction linking logic works for Specviz too, I preserved the original logic for gaussian smooth linking in Cubeviz, while using the Spectral Extraction linking logic for the ndim < 3 usecase.

Fixes #1956

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)?

[duytnguyendtn]

Hmm... Test failed because the Gaussian Smooth plugin couldn't find the autocollapsed 1D spectrum. It's passing locally for me. Can anyone reproduce?

[duytnguyendtn]

@pllim Fresh env installing the merge branch of this PR and testing directly out of site-packages. Test is still passing.

I'm still confused why this is even happening. The autocollapsed spectrum is supposed to generate automatically. Why wouldn't it be available?

[pllim]

The autocollapsed spectrum is supposed to generate automatically. Why wouldn't it be available?

I don't understand that part either but I cannot access the collapsed spectrum in Cubeviz from data_collection. I don't know why you don't see that failure though!

[duytnguyendtn]

And the mystery continues @pllim! Windows and OSX CI is now passing 😱 ! The failure on 3.10 is due to an SSL error

[pllim]

Tsk tsk, the failure is from astroquery, see #2021

[pllim]

Windows and OSX CI is now passing

🤯 Still failing for me locally.

[javerbukh]

I'm in a fresh conda environment and I'm still unable to see the smoothed spectrum in specviz2d. Is that only me?

[kecnry]

I'm in a fresh conda environment and I'm still unable to see the smoothed spectrum in specviz2d. Is that only me?

Make sure you're selecting the 1d spectrum as the input. There is a separate ticket for fixing the 2d spectral smoothing (or maybe for now we should disable the 2D spectrum as input).

[pllim]

Okay, so I know why it fails for me now but I dunno why it doesn't fail for everyone else.

I get UserWarning: background window extends beyond image boundaries (3.5 >= 3) if I raise an error where specviz2d helper emits "Automatic spectrum extraction failed" message.

--- a/jdaviz/configs/specviz2d/helper.py
+++ b/jdaviz/configs/specviz2d/helper.py
@@ -194,6 +194,7 @@ class Specviz2d(ConfigHelper, LineListMixin):
                 try:
                     spext.export_extract_spectrum(add_data=True)
                 except Exception:
+                    raise
                     msg = SnackbarMessage(
                         "Automatic spectrum extraction failed. See the spectral extraction"
                         " plugin to perform a custom extraction",

[duytnguyendtn]

Ahh @pllim I increased the default 2D image size to try and avoid that warning in e8fbab4. Try testing on that commit if you aren't already?

[pllim]

@duytnguyendtn , it passes locally for me now. Thanks!

[pllim]

BTW, CI should pass now if you rebase. FYI.

[javerbukh]

That worked for me @kecnry thanks! I can approve once CI is green.

[pllim]

I still think that pytest fixture should not have any input argument BTW.

[kecnry]

Approving pending the fixture conversation being resolved with @pllim. Thanks for tracking this down!

Does need change log entry and rebasing/passing CI before merge though!

[codecov]

Codecov Report

Base: 92.05% // Head: 92.06% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (68af3c0) compared to base (71a514c).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2023   +/-   ##
=======================================
  Coverage   92.05%   92.06%           
=======================================
  Files         140      140           
  Lines       15287    15305   +18     
=======================================
+ Hits        14073    14091   +18     
  Misses       1214     1214           

Impacted Files	Coverage Δ
jdaviz/app.py	94.42% <100.00%> (ø)
...gins/gaussian_smooth/tests/test_gaussian_smooth.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.

[pllim]

LGTM now, though one remaining nitpick on the name branding? Is it uppercase D or lowercase D?

[pllim]

Suggested change

	* Fixed linking issue preventing smoothed spectrum from showing in Specviz2D. [#2023]
	* Fixed linking issue preventing smoothed spectrum from showing in Specviz2d. [#2023]

[duytnguyendtn]

Docs have it as capital D: https://jdaviz.readthedocs.io/en/latest/specviz2d/index.html

[pllim]

Oh, weird. I thought I remember we agreed to rebrand so that only first letter is capitalized (when we dropped the capital V). Also the class name has lowercase "d".

jdaviz/jdaviz/configs/specviz2d/helper.py

Line 12 in 71a514c

class Specviz2d(ConfigHelper, LineListMixin):

Now I am confused. But I guess this discussion should not block merge. I already approved. Thanks for your patience!

[duytnguyendtn]

Failure is due to unrelated asdf_extension warning in specutils. Merging after consultation with the team. Thanks all!

[lumberbot-app]

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

1. Checkout backport branch and update it.

git checkout v3.3.x
git pull

2. Cherry pick the first parent branch of the this PR on top of the older branch:

git cherry-pick -x -m1 bcb981ebbbacb040d41e5d4f05409685fede19a4

3. You will likely have some merge/cherry-pick conflict here, fix them and commit:

git commit -am 'Backport PR #2023: Fix 2D smooth visibility bug by utilizing SpectralExtraction linking logic'

4. Push to a named branch:

git push YOURFORK v3.3.x:auto-backport-of-pr-2023-on-v3.3.x

5. Create a PR against branch v3.3.x, I would have named this PR:

"Backport PR #2023 on branch v3.3.x (Fix 2D smooth visibility bug by utilizing SpectralExtraction linking logic)"

And apply the correct labels and milestones.

Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove the Still Needs Manual Backport label once the PR gets merged.

If these instructions are inaccurate, feel free to suggest an improvement.