support specreduce 1.3 #1889

kecnry · 2022-12-02T17:55:11Z

Description

This pull request makes necessary changes to support the (upcoming) 1.3 release of specreduce.

This combines (and replaces) #1695 and #1847.

This will

add support for specutils.Spectrum1D objects as input/output to specreduce from Better handled Spectrum1D images across classes astropy/specreduce#144. This will also enable us to support keeping the spectral-axis/WCS throughout the spectral extraction process (although that is temporarily disabled here until the proper plotting support is implemented upstream).
add support for model_type from the FitTrace implementation in Introducing polynomial tracing astropy/specreduce#128.

Todo:

fix units assertion error in test_parser
fix 1d background subtraction mapping to pixels
add validation rules for number of bins
remove temporary specreduce-dev testing

Once specreduce 1.3 is released, this PR should pin that new release and should then be tested (although it does currently seem to work as expected with the current state of the PR branch).

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

pllim · 2022-12-02T18:04:25Z

I think you also need astropy/specreduce#151

kecnry · 2022-12-02T18:17:02Z

that isn't merged yet, so I don't know if it'll make 1.3 or not. What changes would that need on the jdaviz-side? (With the exception of the unit errors that I'm working to address, all tests pass locally)

pllim · 2022-12-02T18:19:18Z

For that test file, the mask returned False which I have to set to None in specreduce. I didn't touch Jdaviz.

pllim · 2022-12-02T18:20:13Z

See #1877 (comment)

pllim · 2022-12-02T18:20:45Z

To be sure, you can temporarily add back dev specreduce job here.

* revert this commit once glue handles non-linear wavelength scaling

pllim · 2022-12-05T20:10:34Z

I don't see any testing against specreduce dev, does this mean specreduce 1.3 is released?

kecnry · 2022-12-05T20:24:43Z

Yes, specreduce 1.3 has been released and pinned as part of this PR. The temporary testing against dev has therefore been removed.

pllim · 2022-12-06T15:01:47Z

jdaviz/configs/specviz2d/plugins/spectral_extraction/spectral_extraction.py


        if add_data:
            self.bg_spec_add_results.add_results_from_plugin(spec, replace=False)

+        # TEMPORARY: override spectral axis to be in pixels until properly supporting plotting


What issue will fix this? Is this future work?

This is the general effort to support wavelengths (waiting for glue/bqplot work in order to sync the axes correctly). So yes, this is intentionally added to keep the current behavior.

pllim

I say we get this in now to get the CI green and worry about little bugs later.

kecnry · 2022-12-06T15:27:35Z

Just to clarify - CI on main/other PRs should not be broken as we pin a minor version of specreduce. This PR just aims to make use of the new functionality and cleaner API in specreduce 1.3 and keep us up-to-date.

pllim · 2022-12-06T15:30:44Z

Hmm... Pretty sure specviz2d auto-collapse failed for me locally so I thought I need this, but maybe my env was dirty... Would you rather not get this in sooner than later?

kecnry · 2022-12-08T18:03:53Z

Found one other place that needs fixing/updating: if using Horne extraction on the original data (not "From Plugin"), then specreduce fails with the following error: image NDData object lacks uncertainty

To reproduce:

from jdaviz import Specviz2d
specviz2d = Specviz2d()

from astroquery.mast import Observations 
fn = "jw01538-o160_s00004_nirspec_f170lp-g235h-s1600a1-sub2048_s2d.fits"
uri = f"mast:JWST/product/{fn}"
result = Observations.download_file(uri)

specviz2d.load_data(fn)
specviz2d.show()

spex = specviz2d.plugins['Spectral Extraction']
spex.open_in_tray()
spex.ext_type = 'Horne'
spex.ext_dataset = 'Spectrum 2D'
spex.export_extract_spectrum()

EDIT: fixed in commit below, marked back as ready-for-review.

codecov · 2022-12-08T18:48:58Z

Codecov Report

Base: 88.45% // Head: 88.48% // Increases project coverage by +0.02% 🎉

Coverage data is based on head (8a2c7a6) compared to base (dd7ffa2).
Patch coverage: 93.75% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1889      +/-   ##
==========================================
+ Coverage   88.45%   88.48%   +0.02%     
==========================================
  Files          95       95              
  Lines       10533    10550      +17     
==========================================
+ Hits         9317     9335      +18     
+ Misses       1216     1215       -1

Impacted Files	Coverage Δ
...plugins/spectral_extraction/spectral_extraction.py	`91.30% <93.75%> (-0.10%)`	⬇️
jdaviz/app.py	`94.57% <0.00%> (ø)`
...imviz/plugins/aper_phot_simple/aper_phot_simple.py	`92.20% <0.00%> (ø)`
jdaviz/core/template_mixin.py	`92.54% <0.00%> (+0.01%)`	⬆️
jdaviz/utils.py	`90.16% <0.00%> (+0.08%)`	⬆️
jdaviz/configs/mosviz/plugins/viewers.py	`87.67% <0.00%> (+0.11%)`	⬆️
...igs/default/plugins/model_fitting/model_fitting.py	`83.87% <0.00%> (+0.70%)`	⬆️

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.

duytnguyendtn

I installed this PR and successfully grabbed specreduce=1.3.0. Running pytest jdaviz resulted in no errors.

My only thought is whether you wanted to add a new test to cover the case you found in #1889 (comment)?

ojustino

The code included so far looks good to me. Was the choice to leave out Chebyshev models intentional? I'll hold off on the approval in case the answer is no because I think it would be good to have parity in the available trace models.

I can get the UI to stop responding for several seconds if I change trace parameters too quickly. Can you replicate that? I don't see any unexpected warnings, and I'm guessing part of the issue is lag from FitTrace's calculation of fits in certain scenarios. (@PatrickOgle already mentioned it at the specreduce repository in a review of polynomial tracing.)

kecnry · 2022-12-12T20:15:43Z

Was the choice to leave out Chebyshev models intentional? I'll hold off on the approval in case the answer is no because I think it would be good to have parity in the available trace models.

I honestly can't remember, but it's trivial to expose (now or later). @PatrickOgle @camipacifici - do we want all supported models in jdaviz or just some subset. I think specreduce's FitTrace currently supports Spline, Polynomial, Legendre, and Chebyshev from astropy/specreduce#128.

I can get the UI to stop responding for several seconds if I change trace parameters too quickly. Can you replicate that? I don't see any unexpected warnings, and I'm guessing part of the issue is lag from FitTrace's calculation of fits in certain scenarios.

Yes, with binning disabled, the interactivity can start to feel laggy. It was largely for this reason that I have binning enabled by default, even though it isn't enabled by default in specreduce. Seconds seems extreme, though, and longer than I've experienced with the sample data I've used for testing - if that is the case, perhaps we should force binning or implement a spinner (not sure if a spinner would be in scope here or a follow-up effort though).

* separate messages shown if binning is disabled or if nbins > 20

kecnry · 2022-12-13T14:55:59Z

@ojustino - I added Chebyshev support and also a warning in the plugin for possible slow behavior. I think I'd rather defer a possible spinner until we see how it performs in the wild.

kecnry · 2022-12-13T15:09:10Z

RTD was passing before and no changes were made to docs since, so I think that is safe to ignore for now.

pllim · 2022-12-13T16:00:48Z

I restarted RTD, just to see if it is transient.

updates for Spectrum1D support in specreduce

3da9aa2

github-actions bot added the specviz2d label Dec 2, 2022

kecnry force-pushed the specreduce-1.3 branch from fff6770 to 0936816 Compare December 2, 2022 19:50

kecnry added 5 commits December 2, 2022 15:26

TEMP: force spectrum to plot in pixel-space

f2c0099

* revert this commit once glue handles non-linear wavelength scaling

update spectral extraction with new model types

0313848

update to make use of FitTrace

9f30a41

binning toggle

81c10ea

update units in parser test

b8bd460

kecnry force-pushed the specreduce-1.3 branch from 0936816 to 75c4660 Compare December 2, 2022 20:27

kecnry added this to the 3.2 milestone Dec 2, 2022

kecnry added 2 commits December 5, 2022 14:22

order-dependent validation for number of bins

72116ed

pin specreduce 1.3

9e2aedc

kecnry force-pushed the specreduce-1.3 branch from 035166b to 9e2aedc Compare December 5, 2022 19:22

kecnry marked this pull request as ready for review December 5, 2022 20:08

kecnry requested review from duytnguyendtn, rosteen, javerbukh, ojustino, pllim and bmorris3 as code owners December 5, 2022 20:08

pllim mentioned this pull request Dec 5, 2022

Improve coordinates display panel for spectrum viewer #1894

Merged

16 tasks

pllim reviewed Dec 6, 2022

View reviewed changes

pllim approved these changes Dec 6, 2022

View reviewed changes

pllim added Ready for final review 🔥 Critical labels Dec 6, 2022

pllim removed the 🔥 Critical label Dec 6, 2022

kecnry marked this pull request as draft December 8, 2022 18:04

re-introduce uncertainty fallbacks for HorneExtract

69574d6

kecnry force-pushed the specreduce-1.3 branch from c371981 to 69574d6 Compare December 8, 2022 18:32

fix minor typo in UI

4dc4aae

kecnry marked this pull request as ready for review December 8, 2022 18:50

duytnguyendtn approved these changes Dec 12, 2022

View reviewed changes

ojustino reviewed Dec 12, 2022

View reviewed changes

kecnry added 2 commits December 13, 2022 09:48

add support for Chebyshev traces

dd4ec24

add UI warning for slow FitTrace

8a2c7a6

* separate messages shown if binning is disabled or if nbins > 20

pllim merged commit cfc2092 into spacetelescope:main Dec 13, 2022

kecnry deleted the specreduce-1.3 branch December 13, 2022 16:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support specreduce 1.3 #1889

support specreduce 1.3 #1889

kecnry commented Dec 2, 2022 •

edited by pllim

Loading

pllim commented Dec 2, 2022

kecnry commented Dec 2, 2022 •

edited

Loading

pllim commented Dec 2, 2022

pllim commented Dec 2, 2022

pllim commented Dec 2, 2022

pllim commented Dec 5, 2022

kecnry commented Dec 5, 2022

pllim Dec 6, 2022

kecnry Dec 6, 2022

pllim left a comment

kecnry commented Dec 6, 2022

pllim commented Dec 6, 2022 •

edited

Loading

kecnry commented Dec 8, 2022 •

edited

Loading

codecov bot commented Dec 8, 2022 •

edited

Loading

duytnguyendtn left a comment

ojustino left a comment

kecnry commented Dec 12, 2022

kecnry commented Dec 13, 2022

kecnry commented Dec 13, 2022

pllim commented Dec 13, 2022

support specreduce 1.3 #1889

support specreduce 1.3 #1889

Conversation

kecnry commented Dec 2, 2022 • edited by pllim Loading

Description

Change log entry

Checklist for package maintainer(s)

pllim commented Dec 2, 2022

kecnry commented Dec 2, 2022 • edited Loading

pllim commented Dec 2, 2022

pllim commented Dec 2, 2022

pllim commented Dec 2, 2022

pllim commented Dec 5, 2022

kecnry commented Dec 5, 2022

pllim Dec 6, 2022

Choose a reason for hiding this comment

kecnry Dec 6, 2022

Choose a reason for hiding this comment

pllim left a comment

Choose a reason for hiding this comment

kecnry commented Dec 6, 2022

pllim commented Dec 6, 2022 • edited Loading

kecnry commented Dec 8, 2022 • edited Loading

codecov bot commented Dec 8, 2022 • edited Loading

Codecov Report

duytnguyendtn left a comment

Choose a reason for hiding this comment

ojustino left a comment

Choose a reason for hiding this comment

kecnry commented Dec 12, 2022

kecnry commented Dec 13, 2022

kecnry commented Dec 13, 2022

pllim commented Dec 13, 2022

kecnry commented Dec 2, 2022 •

edited by pllim

Loading

kecnry commented Dec 2, 2022 •

edited

Loading

pllim commented Dec 6, 2022 •

edited

Loading

kecnry commented Dec 8, 2022 •

edited

Loading

codecov bot commented Dec 8, 2022 •

edited

Loading