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

NSClean Step is not run on background and imprint observations #8591

Closed
stscijgbot-jp opened this issue Jun 24, 2024 · 20 comments · Fixed by #8786
Closed

NSClean Step is not run on background and imprint observations #8591

stscijgbot-jp opened this issue Jun 24, 2024 · 20 comments · Fixed by #8786

Comments

@stscijgbot-jp
Copy link
Collaborator

Issue JP-3667 was created on JIRA by Christian Hayes:

When the NSClean step is turned on in spec2 for reprocessing NIRSpec data as part of an association, the step is only run on the science data and is not run on any other files such as the background or imprint observations.  When these files are used in later steps, they effectively add back in the 1/f noise that NSClean was supposed to clean.  When run on an association NSClean should ideally process the background and imprint members as well as the science members.  The NSClean step also depends on the assign_wcs and msa_flagging steps, so these steps would need to be run on the background an imprint files.

@stscijgbot-jp
Copy link
Collaborator Author

Comment by Christian Hayes on JIRA:

David Law do you know who would be best to check with about this issue?

@stscijgbot-jp
Copy link
Collaborator Author

Comment by Melanie Clarke on JIRA:

I'm starting work on 1/f implementation for the next build (JP-3639).  I've been wondering what we want to do with the current nsclean step anyway, so maybe I can help?

@stscijgbot-jp
Copy link
Collaborator Author

Comment by Christian Hayes on JIRA:

Thanks Melanie Clarke, I did take a look at what is being done in spec2 prior to running nslean, and had some ideas, but have not had a chance to put together/propose a solution, so I'd be happy to chat if it would be useful.  I wasn't sure whether the background/imprint members should also be run through some of the earlier steps in spec2 proper (which wouldn't really be needed in the default pipeline) or if it would make more sense to check whether these steps had been run in the nsclean step and run them if mask_spectral_regions=True.  

I think we would want to keep the current nsclean step at least for a little while, but it might also be useful to have some known source region masking in the JP-3639 1/f implementation if it is including nsclean based algorithm that is close to the current spec2 step.  Though I understand that the goal is to get a working version first before adding features, so that might be something to think about later.

@stscijgbot-jp
Copy link
Collaborator Author

Comment by David Law on JIRA:

Hm, documentation disagrees at to when nsclean is meant to run in the spec2 workflow.  According to JDox, nsclean runs after background and imprint subtraction but before MSA flagging.  According to readthedocs (and an actual pipeline run) nsclean happens after msa flagging but before imprint and background subtraction.

So, which is the actual intended order?  If background and imprint happen before nsclean, then presumably that might resolve the issue?

@stscijgbot-jp
Copy link
Collaborator Author

stscijgbot-jp commented Jul 18, 2024

Comment by Melanie Clarke on JIRA:

NSClean does indeed run after MSA flagging but before imprint and background subtraction.  I think that was the original intent, to correct the 1/f noise due to the file itself – background and imprint noise sources were just overlooked.

We could instead move it after background and imprint subtraction, to correct the noise added from all sources.  This is maybe less rigorous, but would be simpler to implement.  Since we're planning on adding a more rigorous correction option at the group level in detector1, maybe this would be sufficient for the cosmetic clean up in spec2?

@stscijgbot-jp
Copy link
Collaborator Author

Comment by David Law on JIRA:

Perhaps let's compare and see how it works?  Chris, could you see what the difference is running nsclean once on data that's been background/imprint subtracted vs background/imprint subtracting data that have all been through nsclean individually?  Given that we're working on getting this running at the group stage anyway I'd be hesitant to put substantial effort into rethinking the spec2 version right now.

@stscijgbot-jp
Copy link
Collaborator Author

Comment by Christian Hayes on JIRA:

David Law Melanie Clarke, yes I'll look into applying nsclean after background subtraction.  We discussed this briefly on the NIRSpec team and while it might be less rigorous it was also pointed out that this might also mean fewer opportunities for something to go wrong with applying nsclean multiple times on each background rate (plus applying imprint and background subtraction might help remove additional light from dark regions of the detector).

@stscijgbot-jp
Copy link
Collaborator Author

Comment by Christian Hayes on JIRA:

The NIRSpec team has looked into using nsclean on background subtracted rates.  With the spectral region masking, the differences between cleaning background subtracted rates and cleaning input rate files before performing background subtraction is very small within the regions that are used for science.  However there are considerable differences that can arise when using sigma clipping only (the image that shows examples with sigma=1), because the sigma clipping only removes positive sources from the mask and not the negative traces from subtracted nods. I've attached two examples showing these cases.  Therefore, the preference would be to apply nsclean on all input rate files rather than the background subtracted rates

The current limitation for running NSClean on background/imprint association members is that they do not have the assign_wcs and msa_flagging steps run on them in the spec2 pipeline, which is needed for the NSClean step.  The new clean_flicker_noise step from https://jira.stsci.edu/browse/JP-3639 (and the update to NSClean to alias this step) calls the assign_wcs and msa_flagging steps internally, which would allow it to be run on all association members in spec2.  If it sounds reasonable, the NIRSpec preference would be to use the new step and modify spec2 to run NSClean on all asn members once this is possible (though cleaning all input files at the group stage may still be better).  

Melanie Clarke I have an example of using the new clean_flicker_noise step to do this in spec2 and I'm happy to open a new PR once the JP-3639 PR ([https://github.com//pull/8669]) has been merged, unless it would be reasonable to add it to the current PR.

@stscijgbot-jp
Copy link
Collaborator Author

Comment by Melanie Clarke on JIRA:

Thanks for testing Christian Hayes - let's keep this work separate from the JP-3639 PR, and add it in after that one is merged.

@stscijgbot-jp
Copy link
Collaborator Author

Comment by Melanie Clarke on JIRA:

Christian Hayes - JP-3639 is still not quite merged, but should be soon.  Do you want to try to get in this fix before the build closes at the end of next week?

@stscijgbot-jp
Copy link
Collaborator Author

Comment by Christian Hayes on JIRA:

Melanie Clarke ideally yes, and I think the changes should be reasonably straightforward (just adding loops over background/imprint members in an asn to run NSClean on them), but if it can't be done, the changes to NSClean in JP-3639 will at least make workarounds simpler and we could add this to the next build.

@stscijgbot-jp
Copy link
Collaborator Author

Comment by Melanie Clarke on JIRA:

Okay, let's give it a try!  Do you want to put in the PR or should I?

@stscijgbot-jp
Copy link
Collaborator Author

Comment by Christian Hayes on JIRA:

Into the JP-3639 PR? I'd be happy to, but whatever would be easiest on your end.

@stscijgbot-jp
Copy link
Collaborator Author

Comment by Melanie Clarke on JIRA:

Sorry, no, I think it should be a separate PR, but we can branch it off of the JP-3639 work, assuming that one will go in first.  I'll reach out offline so we can sort it out.

@stscijgbot-jp
Copy link
Collaborator Author

Comment by Christian Hayes on JIRA:

Thanks Melanie Clarke, sorry I had misread your comment, but that makes sense.  I've opened up a draft PR here:  https://github.com/spacetelescope/jwst/pull/8786], which is branched off of the JP-3639 work.  See the last two commits for the changes relevant to this ticket (and I'll clean up the history once the clean_flicker_noise PR has been merged).  

For completeness I've attached two examples using the changes in this PR [^jw02756001001_03103_00003_nrs2_nsclean_bkg_members.png] and [^jw1328036001_02101_00001_nrs2_nsclean_imprint_members.png], which show the imprint/bkg subtracted rate files from an association processed through the current version of the pipeline (old - left) and processed with this PR (new - right), with nsclean on in both cases.  The old version shows 1/f noise reintroduced from the imprint and background members, whereas these rate files are cleaned before subtraction with the new version.

@stscijgbot-jp
Copy link
Collaborator Author

Comment by Melanie Clarke on JIRA:

Thanks, that looks good to me, and straightforward enough.  I'll run tests for it after we get JP-3639 in.

Of note - as written, the same nsclean parameters will be applied to all of the background, imprint, and science exposures, including the same user mask if supplied.  I assume that's okay? It's still possible to work around it and correct them individually if desired.

@stscijgbot-jp
Copy link
Collaborator Author

Comment by Christian Hayes on JIRA:

Sounds good.

Yes, that was the idea.  I think if users want to run 1/f cleaning with different parameters/user supplied mask/etc. it's probably best to do so separately for each file rather than try to set separate step parameters for the science and imprints/backgrounds.   This might not be perfect in all cases, but I think using the same parameters should work reasonably well for general cases and like you mentioned there are straightforward workarounds available if something more custom is desired.

@stscijgbot-jp
Copy link
Collaborator Author

Comment by Melanie Clarke on JIRA:

Fixed by #8786

@stscijgbot-jp
Copy link
Collaborator Author

Comment by Christian Hayes on JIRA:

PR fixes this issue, thanks Melanie for helping get this in in time for the build.  

Closing.

@stscijgbot-jp
Copy link
Collaborator Author

Comment by Melanie Clarke on JIRA:

A minor follow-up fix in #8809

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant