JP-3566 Move pixel replace before resample/cube build

[jemorrison]

Resolves JP-3566

Closes #8340

This PR moves the pixel replace step to right before resample/cube build in both the Spec2 and Spec3 pipelines. The *cal.fits files now do not include the replace pixel values. These replacements are occurring right before resample/cubebuild.

Checklist for maintainers

• 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
  as of 5/22/3:30 the latest regression tests are at
  https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/1470/
• Make sure the JIRA ticket is resolved properly

[jemorrison]

@tapastro
This PR has how I was planning on moving the pixel replacement. I have not tested it yet. I just wanted to check it this method seems like the right approach before I move into testing. The cal files would not longer include the replaced pixels.

[drlaw1558]

@jemorrison That looks about right to me. Presumably the standard save_results=True approach will make the pix replace step write output files for test/debugging purposes?

[codecov]

Codecov Report

Attention: Patch coverage is 10.66667% with 67 lines in your changes are missing coverage. Please review.

Project coverage is 57.91%. Comparing base (4179c09) to head (ff0d5fa).
Report is 3 commits behind head on master.

❗ Current head ff0d5fa differs from pull request most recent head d3c24f0

Please upload reports for the commit d3c24f0 to get more accurate results.

Files	Patch %	Lines
jwst/pixel_replace/pixel_replace_step.py	2.63%	37 Missing ⚠️
jwst/pipeline/calwebb_spec3.py	13.63%	19 Missing ⚠️
jwst/pipeline/calwebb_spec2.py	30.00%	7 Missing ⚠️
jwst/pipeline/calwebb_tso3.py	25.00%	3 Missing ⚠️
jwst/pixel_replace/pixel_replace.py	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8409      +/-   ##
==========================================
- Coverage   57.97%   57.91%   -0.07%     
==========================================
  Files         387      387              
  Lines       38830    38882      +52     
==========================================
+ Hits        22513    22518       +5     
- Misses      16317    16364      +47     

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

[jemorrison]

@tapastro The pixel replace step assumed the input data was a single input not a model container since it was written for calspec2. Now the pixel_replacement step needs to also work over ModelContainers.
Take a look at the changes I made to pixel_replace_step.py and see if they look correct to you.

In calspec2 the code it works as expected. In the cal file, bad pixels have a science value of nan. If I save the pixel_replace output then the science frame has the nans replaced (so now these files look like the old ca files). The files names are expected.

In calpsec3 the code works fine. The output filenames when pixel_replace.save_results=True are not what I was expecting. I need to make the name look more like the output from outlier detection, for example:
jw01204018001_02101_00004_mirifushort_ratesub_o018_crf.fits - where the o018 is the asn_id
I tried to replicate the outlier_detection code using the asn_id and make_output_path - but I do not have it right.
The filename are of the form:
jw01204-o018_t003_miri_7_pixel_replace.fits

Not sure what I am doing wrong.

[jemorrison]

@emolter @hbushouse
I have to code working as intended. I just can not get the pixel_replace output filenames to be correct.
This PR moves where pixel_replace is called. When it is called in calspec3 the output names should be similar to the output names for outlier detection.
For example:
jw01204018001_02101_00001_mirifulong_o018_crf.fits
but for pixel replace I have jw01204-o018_t003_miri_0_pixel_replace.fits

I looked at what outlier detection was doing for file names and I can get the correct looking file name and I print it in the step (I left the print statement in the code for now - to show you where it works). But the final resulting output names are the same. Hum ? What I am missing ?

Snipet of code:
self.log.debug('Input is an IFUImageModel.')
replacement = PixelReplacement(model, **pars)
replacement.replace()
self.record_step_status(replacement.output, 'pixel_replace', success=True)
output_model[i] = replacement.output
o_path_name = self.make_output_path(basepath=output_model[i].meta.filename)
print(o_path_name). ****** THIS IS THE FILENAME I WANT
output_model[i].meta.filename = o_path_name

[emolter]

It looks like the style checks are failing:
jwst/pixel_replace/pixel_replace_step.py:6:29: F401 [*] jwst.datamodels.ModelContainer imported but unused
jwst/pixel_replace/pixel_replace_step.py:101:46: F821 Undefined name partial

The second one of these makes me worried that there is no test coverage for the renaming, because this seems like it should cause failures. Is there a way to write a unit test that ensures the output filenames are as expected given association and non-association inputs?

[jemorrison]

@hbushouse
Regression tests:
Several updates to regression test will cause failures. I added pixel_replace.skip=False for MIRI LRS SLIT and SLITLESS
since that is default now. All those down stream results will need to be okify.
I added to the truth files when no file existed (*pixel_replace.fits and *photom.fits. I added the photom results because they are missing from the tests and don't think they should be.)

For MIRI IFU , NIRSpec IFU and FS I added new test that ran pixel_replace, since at this time pixel_replace.skip=True in the pipeline for those modes. I think that might change in the future, so I just added another test rather than changing the main test. For these new tests I uploaded all the truth files.

I do not have a test for NIRSpec MOS data. I hope having all the other tests is good enough.

https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/1417/

There are main failures - several of them are related to Extra keyword 'S_PXREPL' in b: 'SKIPPED'. This is when pixel_replace is not run on the data.
I do not have a test for NIRSpec MOS data. I don't know if that is a mode that will use with step.

If you want me to I can go through the results and okify them. I just need a refresher on how that is done.

[emolter]

Code changes look fine to me. There's probably some way to refactor pixel_replace_step to avoid the long if...else chain but I don't think that's very important.

Should I be worried about the bigdata error that the regtest run can't find certain files in Artifactory? e.g. this one?

[hbushouse]

Started yet another regtest run at https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/1483

[hbushouse]

Tally from latest regtest run (1483):

1. test_miri_lrs_slit_spec2::test_miri_lrs_extract1d_from_cal shows no S_PXREPL keyword in extract_1d output products, because the test is not calling the pixel_replace step at all, whereas in the past it had been applied to the cal file used as input to the test (it no longer is).
2. cal, calints, crf, and crfints products no longer contain the S_PXREPL keyword, because the step is now called after those products are created, and hence there are some differences in pixel values as well.
3. s2d, s3d, x1d, and x1dints products from tests where the default pixel_replace.skip=True is used are showing S_PXREPL="SKIPPED", as expected.

All of the above are as expected and I believe confirms that this is all now working as expected.

[hbushouse]

I declare success!