JP-3657: Part 1 of a NIRSpec BOTS speedup

[t-brandt]

Partially resolves JP-3657

Partially addresses #8559

This PR enables the reuse of aperture correction objects for spectroscopic data with multiple integrations, specifically targeted at BOTS data. Most of the time in a BOTS reduction to x1d files goes into constructing multidimensional interpolating splines for every wavelength and for every integration. Reusing these across integrations saves a factor of ~10 in BOTS runtime. A further factor of maybe 2 or 3, larger if a background is fit, will come from a rewrite of extract1d (hence this PR only partially addresses the JIRA ticket).

Checklist for PR authors (skip items if you don't have permissions or they are not applicable)

• added entry in CHANGES.rst within the relevant release section
• updated or added relevant tests
• updated relevant documentation
• added relevant milestone
• added relevant label(s)
• ran regression tests, post a link to the Jenkins job below.
  How to run regression tests on a PR
• All comments are resolved
• Make sure the JIRA ticket is resolved properly

[codecov]

Codecov Report

Attention: Patch coverage is 8.00000% with 46 lines in your changes missing coverage. Please review.

Project coverage is 60.16%. Comparing base (2f4410f) to head (ebd929c).

Files with missing lines	Patch %	Lines
jwst/extract_1d/apply_apcorr.py	11.53%	23 Missing ⚠️
jwst/extract_1d/extract.py	4.16%	23 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8609      +/-   ##
==========================================
- Coverage   60.21%   60.16%   -0.05%     
==========================================
  Files         370      370              
  Lines       38630    38666      +36     
==========================================
+ Hits        23261    23264       +3     
- Misses      15369    15402      +33     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[melanieclarke]

A few suggestions below, mostly tiny style suggestions and typo fixes, but also one bug fix for extended sources that don't go through the aperture correction clause.

It does also seem like it would be cleaner, with less repeated code, to move the tabulated correction into the apcorr.apply method instead of making it a separate function.

Would it be possible to add a unit test to verify that tabulated and un-tabulated corrections are the same?

[t-brandt]

I refactored the code slightly so that apcorr.apply() takes use_tabulated as an (optional) argument. Please tell me if you think this approach is better. If so, we can double-check the test results.

I think we should be able to write a unit test to verify that calling tabulate_correction and .apply(use_tabulated=True) gives the same results as .apply(usetabulated=False). It would be best to do this on a data set with a nontrivial aperture correction.

[melanieclarke]

I refactored the code slightly so that apcorr.apply() takes use_tabulated as an (optional) argument. Please tell me if you think this approach is better. If so, we can double-check the test results.

These changes look sensible to me - thanks!

I'll be out next week, but can check back in after if there is still testing work that needs to be done.

[t-brandt]

Adding a unit test to verify that the tabulated and non-tabulated corrections are the same is harder than I thought. This will require a new dummy data set (since none of the ones there currently have the keys in the data tables). I was hoping it would just be a couple of lines. I could generate a data set and add it to the repository, and then write a test to verify that the answer is the same in the various ways of computing an aperture correction. Should I do this?

[melanieclarke]

Adding a unit test to verify that the tabulated and non-tabulated corrections are the same is harder than I thought. This will require a new dummy data set (since none of the ones there currently have the keys in the data tables). I was hoping it would just be a couple of lines. I could generate a data set and add it to the repository, and then write a test to verify that the answer is the same in the various ways of computing an aperture correction. Should I do this?

I'm not very familiar with the current apcorr tests. Is it possible to just add the keys you need to the existing mock data sets?

[t-brandt]

The current tests simply check that the interpolating aperture correction function is unity at a random wavelength (i.e. that there is no correction). A proper test, to my mind, for the current change is that the aperture correction applied to a grid of wavelengths is the same whether applied in the old or in the new way. The function that applies the correction has a variety of fields hard-coded in in the form of a FITS table that it assumes as input; such a table is never actually called or referenced in the current checks. So I think it would be a bit of a pain to implement and would require a new dummy reference file. If it is important to do this I can give it a go.

[drlaw1558]

It may be simpler then to cover this in the regression test framework (which should happen automatically) rather than as a unit test.

[melanieclarke]

I see, the current unit tests don't cover any of this functionality yet. I agree it would take some development time to work up appropriate mock test data. I think it would be nice to have, but it's not critical for this change.

[melanieclarke]

I think all comments have been addressed for this PR. Regression tests are started here:
https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/1617/

[melanieclarke]

No failures at all in the regression tests! I'm not sure I've ever seen that before.

LGTM!