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

NaN fix for negative CCD bkg #1927

Merged
merged 1 commit into from Dec 2, 2022
Merged

NaN fix for negative CCD bkg #1927

merged 1 commit into from Dec 2, 2022

Conversation

sbailey
Copy link
Contributor

@sbailey sbailey commented Dec 2, 2022

This PR fixes a problem in desispec.preproc.compute_background_between_fiber_blocks where a row with a completely negative background resulted in the median of an empty array, causing the background model to have NaNs, which then broke things downstream like traceshift fitting.

Example of this case occurring:

desi_preproc -i /global/cfs/cdirs/desi/spectro/data/20221126/00154927/desi-00154927.fits.fz \
  --cameras b4 --bkgsub-for-science   --model-variance \
  --bias /global/cfs/cdirs/desi/spectro/redux/daily/calibnight/20221126/biasnight-b4-20221126.fits.gz \
   -o ./preproc-b4-00154927.fits.gz

In current main, this results in NaN for row 3247 from columns 453-836 (spanning two bundles):
image
in this PR these the bkg is interpolated and subtracted without artifacts:
image

I think the original code was just because it was written to subtract positive scattered light and wasn't considering the negative case, but I think it's ok to let this absorb negative problems with the bias model too. @julienguy please check this logic for whether there might be other consequences that I'm not realizing.

Side note: this PR is into main because I was working on Perlmutter, but if approved we should also make this change to the daily branch for use on cori.

Copy link
Contributor

@julienguy julienguy left a comment

Choose a reason for hiding this comment

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

That's a bug I should not have done. Good catch.

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

Successfully merging this pull request may close these issues.

None yet

2 participants