Codecov Report

Attention: Patch coverage is 14.28571% with 18 lines in your changes missing coverage. Please review.

Project coverage is 64.51%. Comparing base (208e7ce) to head (5756f31).
Report is 8 commits behind head on main.

@@            Coverage Diff             @@
##             main    #8979      +/-   ##
==========================================
- Coverage   64.53%   64.51%   -0.03%     
==========================================
  Files         375      375              
  Lines       38728    38744      +16     
==========================================
+ Hits        24993    24995       +2     
- Misses      13735    13749      +14     

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

@melanieclarke @emolter
As I am writing a unit test for this step I have come across something I want to check with you guys.
After running pixel_replace on a single file the output data has 'pixelreplacestep' in the output.meta.filename.
But if I run it on a container the returned output is the input cal file. BUT if I write the data out with save_results=True. The correct filename (with pixelreplacestep) is on disk.
I am testing this:
file = []
file.append('jw01267003001_02105_00001_mirifushort_cal.fits')
file.append('jw01267003001_02105_00002_mirifushort_cal.fits')
pixel_replace = pixel_replace_step.PixelReplaceStep()
container = ModelContainer(file)
pixel_replace.call(container, save_results=True, skip=False)
for c in container:
print(c.meta.filename)
Here is a snap shot of running the step on a container. The information in the log show the correct filenames are created but the print statement shows the return value in meta.filename is the cal file

024-11-27 07:10:50,422 - stpipe.PixelReplaceStep - INFO - Saved model in jw01267003001_02105_00001_mirifushort_pixelreplacestep.fits
2024-11-27 07:10:51,059 - stpipe.PixelReplaceStep - INFO - Saved model in jw01267003001_02105_00002_mirifushort_pixelreplacestep.fits
2024-11-27 07:10:51,060 - stpipe.PixelReplaceStep - INFO - Step PixelReplaceStep done
2024-11-27 07:10:51,060 - stpipe - INFO - Results used jwst version: 1.16.1.dev237+g59ef24d68

jw01267003001_02105_00001_mirifushort_cal.fits
jw01267003001_02105_00002_mirifushort_cal.fits

So the step is working just fine but I can not seem to access the output filenames when it is called from a container.
Any suggestions ? Frankly it works so that is a win. I just wanted the filenames to have the pixelreplacestep in the name so I could have a unit test

@melanieclarke I got this step to work by including the 'invariant_filenames' routine that is from cal spec3 and it manages filenames. I just copied the routine and suck in pixel_replace_step.py. I tried to import it from:
from jwst.pipeline.calwebb_spec3 import invariant_filename

But that gets circular and I get an error because calwebb_spec3 is importing pixel_replace.
I can leave it as it is - but maybe you have a better idea rather than having duplicate code.

So the step is working just fine but I can not seem to access the output filenames when it is called from a container.
Any suggestions ? Frankly it works so that is a win. I just wanted the filenames to have the pixelreplacestep in the name so I could have a unit test

The invariant_filename wrapper appears to ensure that model.meta.filename does NOT change when step.save_model is called. I'm not convinced that having _pixelreplacestep in model.meta.filename is the right thing to do, because isn't that just outdated again once the container is passed to the next step?

I didn't review this in detail yet but I don't see anything wrong with the current solution.

For unit testing, instead of checking the model.meta.filename, could you check the actual name of the files written to disk? See e.g. this outlier detection unit test as an example - it just uses assert path.isfile(filename). Make sure you pass in the tmp_cwd fixture like this test does, too.

But that gets circular and I get an error because calwebb_spec3 is importing pixel_replace.
I can leave it as it is - but maybe you have a better idea rather than having duplicate code.

We could move that function to a library and import it in both places - maybe in lib/pipe_utils.py. I'm not sure I understand why that's working, though.

We could move that function to a library and import it in both places - maybe in lib/pipe_utils.py.

At risk of nit-picking, since invariant_filename wraps an stpipe.step method, would somewhere within jwst.stpipe be a good place for it?

Testing locally, it looks like this solution does work for the case we were trying to fix, where container is made from a list of filenames:
PixelReplaceStep.call(container, save_results=True, skip=False)

But it now un-fixes the calwebb_spec3 case. Calling like this:
strun calwebb_spec3 s3.json --steps.pixel_replace.skip=False --steps.pixel_replace.save_results=True
the output from pixel_replace is correct, but the final products look like this:
_g395h-f290lp_s3d.fits and _g395h-f290lp_x1d.fits.

@melanieclarke @emolter I do have a question about a unit test that saves the output to disk. Is that allowed ? It writes out files to the test directory ?

@melanieclarke I need to understand your test better. In your s3.json file what is the output name set to ?

I have a set of tests (in a Jupyter notebook) that I have developed to test the various filenames of pixel_replace and cube_build. When I run calspec3 and save the pixel_replace output. ALL the filenames have the correct format.

spec3_asn = 'test.json'
spec3 = Spec3Pipeline()
result = spec3.call(spec3_asn,
steps={ 'outlier_detection':{'skip':True},
'pixel_replace':{'skip': False, 'save_results': True},
'cube_build':{'save_results':True},
'extract_1d':{'skip': True}
},
save_results=True)

in the association the name is set to 'test_miri' and there are 4 input files.
The output from pixel_replace is:
024-11-27 11:37:44,322 - stpipe.Spec3Pipeline.pixel_replace - INFO - Saved model in jw01267003001_02105_00002_mirifushort_ctest_pixel_replace.fits
2024-11-27 11:37:44,765 - stpipe.Spec3Pipeline.pixel_replace - INFO - Saved model in jw01267003001_02105_00001_mirifushort_ctest_pixel_replace.fits
2024-11-27 11:37:45,482 - stpipe.Spec3Pipeline.pixel_replace - INFO - Saved model in jw01267003001_02105_00004_mirifushort_ctest_pixel_replace.fits
2024-11-27 11:37:45,936 - stpipe.Spec3Pipeline.pixel_replace - INFO - Saved model in jw01267003001_02105_00003_mirifushort_ctest_pixel_replace.fits

and the output from cube_build is:
2024-11-27 11:38:17,804 - stpipe.Spec3Pipeline.spectral_leak - INFO - Saved model in test_miri_ch2-short_x1d.fits
2024-11-27 11:38:17,914 - stpipe.Spec3Pipeline.spectral_leak - INFO - Saved model in test_miri_ch1-short_x1d.fits

@melanieclarke Forget last message. I was on a wrong branch. Let me check what is going on with cube_build and I will update

@emolter I do have a question about a unit test that saves the output to disk. Is that allowed ? It writes out files to the test directory ?

If you're passing in e.g. the tmp_cwd fixture to the test, it will make its own temporary directory where it writes out files. After the test ran, you should not see any new files in the test directory.