Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JP-3235: Implement NSClean step for NIRSpec 1/f noise #8000

Merged
merged 40 commits into from
Dec 5, 2023

Conversation

hbushouse
Copy link
Collaborator

@hbushouse hbushouse commented Oct 10, 2023

Resolves JP-3235

Closes #7724

This PR implements the "NSClean" algorithm for removing 1/f noise from NIRSpec IFU and MOS images. This initial commit is a crude first draft that just implements basic step structure and functionality. More work to come, especially specifics for handling MOS vs. IFU.

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
  • Make sure the JIRA ticket is resolved properly

@hbushouse hbushouse added the nsclean NIRSpec NSClean algorithm label Oct 10, 2023
@hbushouse hbushouse added this to the Build 10.1 milestone Oct 10, 2023
@hbushouse hbushouse requested a review from a team as a code owner October 10, 2023 19:59
@hbushouse hbushouse marked this pull request as draft October 10, 2023 20:00
@codecov
Copy link

codecov bot commented Oct 10, 2023

Codecov Report

Attention: 274 lines in your changes are missing coverage. Please review.

Comparison is base (00110b3) 75.94% compared to head (b451cd6) 75.41%.

Files Patch % Lines
jwst/nsclean/lib.py 9.33% 136 Missing ⚠️
jwst/nsclean/nsclean.py 11.67% 121 Missing ⚠️
jwst/nsclean/nsclean_step.py 47.05% 9 Missing ⚠️
jwst/pipeline/calwebb_spec2.py 11.11% 8 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8000      +/-   ##
==========================================
- Coverage   75.94%   75.41%   -0.53%     
==========================================
  Files         460      464       +4     
  Lines       37629    37941     +312     
==========================================
+ Hits        28577    28615      +38     
- Misses       9052     9326     +274     
Flag Coverage Δ *Carryforward flag
nightly 77.37% <ø> (-0.01%) ⬇️ Carriedforward from 00110b3

*This pull request uses carry forward flags. Click here to find out more.

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

@hbushouse hbushouse changed the title JP-3235: Implement NSClean step for NIRSpec 1/f noise WIP: JP-3235: Implement NSClean step for NIRSpec 1/f noise Oct 18, 2023
@hbushouse
Copy link
Collaborator Author

@melanieclarke I think this is now ready for a final review. I've tested the latest updates on subarray images, both simple 2D versions and 3D BOTS, and it appears to work.

@hbushouse
Copy link
Collaborator Author

@melanieclarke Whoops, just noticed that I need to go back and update the step docs to remove the warnings/notes about not being able to work on subarrays. I'll take care of that. But other than that, the actual code is final (I hope!).

@melanieclarke
Copy link
Collaborator

Thanks @hbushouse! I should have some time to review later this week.

@melanieclarke
Copy link
Collaborator

@hbushouse - sorry for the delay, I'm testing a number of things now and will pass on input when I have it.

For starters - things look good so far for subarray support and opting out of masking the spectral regions. Thanks for the updates!

The user mask support needs a little clean up (above), but making those changes locally made the rest of it work fine with a modified version of a mask saved by an earlier run.

Also, attempting to clean an ALLSLITS subarray (e.g. jw01128004001_0310g_00003_nrs1_rate.fits) consistently uses up all the memory on my machine, then kills the kernel. This is true for the original NSClean algorithm too, and I don't know why. Can we either try to fix it or else skip cleaning for this mode for now?

I have not yet tested BOTS -- I'll let you know how it goes.

@hbushouse
Copy link
Collaborator Author

Also, attempting to clean an ALLSLITS subarray (e.g. jw01128004001_0310g_00003_nrs1_rate.fits) consistently uses up all the memory on my machine, then kills the kernel. This is true for the original NSClean algorithm too, and I don't know why. Can we either try to fix it or else skip cleaning for this mode for now?

Huh, interesting. Is there anything particularly weird about that dataset? (e.g. lots of masked pixels or something). Have you tried any other ALLSLITS examples? Can't imagine why that particular subarray shape/size would cause unique problems.

I have not yet tested BOTS -- I'll let you know how it goes.

I tried it on one and it runs without failing, but my particular example had so many flagged/masked pixels it's hard to tell whether it successfully fit and removed the background noise.

@melanieclarke
Copy link
Collaborator

It happens for every ALLSLITS data set I've tried. Maybe something about the data array shape with the subarray implementation?

I tested on a BOTS file with 1512 integrations. It worked successfully, but masking the spectral trace area leaves very little unilluminated data, so the correction is poor. It works better with just sigma-clipping, but then it takes some experimentation to get the right n_sigma. It took about 1.5-2 hours per cleaning run on my computer, depending on my settings, so cleaning BOTS data with this algorithm will take some dedication on the user's part.

@melanieclarke
Copy link
Collaborator

melanieclarke commented Nov 30, 2023

The ALLSLITS memory issue is because of the subarray width in the x-direction (in detector space). Subarrays are currently handled by inverting the FFT matrix for the full array all at once, where the full-frame mode proceeds line by line. The extra size for ALLSLITS makes the matrix too big to fit in memory when handled by the subarray case.

I looked briefly into whether it would be possible to support ALLSLITS with the full-frame mode or to convert the subarray mode to work one line at a time, but there was no obvious way to make either option work. It needs more study, and maybe some input from Bernie on the frequency filtering methods that differ between the two modes.

For this first implementation, I suggest we disable NSClean for ALLSLITS subarrays and file a ticket to implement it later and/or merge the subarray and full-frame handling.

@hbushouse
Copy link
Collaborator Author

@melanieclarke This is ready for final review and/or testing. The only recent change was to add an abort if someone feeds in an ALLSLITS subarray, with an appropriate warning in the docs saying that it doesn't work for ALLSLITS. Given that that's all that's changed, if you were satisfied with your last round of tests on the modes that are supported, then I think we're good to go.

Note that I've still got the skip parameter in the step module hardwired to skip, so that this will not get executed in normal processing even after it's merged.

Copy link
Collaborator

@melanieclarke melanieclarke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked the user mask implementation, the ALLSLITS handling, and the default to skip the step, and I think everything looks good now. Thanks again!

Copy link
Collaborator

@nden nden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not correct Python types everywhere. Change boolean --> bool, string --> str

jwst/nsclean/nsclean.py Outdated Show resolved Hide resolved
jwst/nsclean/nsclean.py Outdated Show resolved Hide resolved
jwst/nsclean/nsclean.py Outdated Show resolved Hide resolved
jwst/nsclean/nsclean.py Outdated Show resolved Hide resolved
jwst/nsclean/nsclean.py Outdated Show resolved Hide resolved
jwst/nsclean/nsclean.py Outdated Show resolved Hide resolved
jwst/nsclean/nsclean.py Outdated Show resolved Hide resolved
jwst/nsclean/nsclean.py Outdated Show resolved Hide resolved
jwst/nsclean/nsclean_step.py Outdated Show resolved Hide resolved
jwst/nsclean/nsclean_step.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@nden nden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This clean method needs some cleaning

jwst/nsclean/lib.py Outdated Show resolved Hide resolved
jwst/nsclean/lib.py Outdated Show resolved Hide resolved
jwst/nsclean/lib.py Outdated Show resolved Hide resolved
jwst/nsclean/lib.py Outdated Show resolved Hide resolved
jwst/nsclean/lib.py Outdated Show resolved Hide resolved
jwst/nsclean/lib.py Show resolved Hide resolved
jwst/nsclean/lib.py Outdated Show resolved Hide resolved
jwst/nsclean/lib.py Outdated Show resolved Hide resolved
jwst/nsclean/nsclean.py Outdated Show resolved Hide resolved
jwst/nsclean/nsclean.py Outdated Show resolved Hide resolved
@hbushouse
Copy link
Collaborator Author

@nden Are we happy now?

@hbushouse hbushouse merged commit cd9c19e into spacetelescope:master Dec 5, 2023
26 of 30 checks passed
@hbushouse hbushouse deleted the jp-3235 branch December 5, 2023 20:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NIRSpec improved 1/f noise, pedestal, picture frame handling
4 participants