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

Sky subtraction bug fix #1895

Merged
merged 2 commits into from Nov 3, 2022
Merged

Sky subtraction bug fix #1895

merged 2 commits into from Nov 3, 2022

Conversation

julienguy
Copy link
Contributor

This PR fixes a bug introduced recently in PR #1886 where the amplitude correction was applied twice when cosmic_nsig>0 .

The changes are :

  • modify and subtract a copy of the skymodel.flux array in subtract_sky
  • do not run twice the sky subtraction when flagging cosmic rays because this is not necessary. this was justified when the throughput corrections were computed during the sky subtraction but it is not the case any more.

I will rerun the tiles of PR #1886 to verify this does not cause new problems.

@julienguy
Copy link
Contributor Author

Tested a few tiles (bright and dark) showing no surprise (below comparison with daily):
tile=3377 (dark) efftime=981.7 frac(|dz|/(1+z)>0.003)=0.0000 nz(new)/nz(old)=3415/3413=1.0006
tile=4974 (dark) efftime=1142.6 frac(|dz|/(1+z)>0.003)=0.0000 nz(new)/nz(old)=3400/3406=0.9982
tile=20709 (bright) efftime=213.6 frac(|dz|/(1+z)>0.003)=0.0000 nz(new)/nz(old)=3898/3898=1.0000
tile=22259 (bright) efftime=216.2 frac(|dz|/(1+z)>0.003)=0.0000 nz(new)/nz(old)=3840/3840=1.0000
tile=22428 (bright) efftime=214.6 frac(|dz|/(1+z)>0.003)=0.0000 nz(new)/nz(old)=4128/4128=1.0000
tile=23331 (bright) efftime=197.7 frac(|dz|/(1+z)>0.003)=0.0000 nz(new)/nz(old)=3935/3937=0.9995

Nearly identifical redshift performance. Slightly improved best fit redrock chi2 (here difference between best fit redrock chi2 with this PR compared to daily):
chi2

Copy link
Contributor

@sbailey sbailey left a comment

Choose a reason for hiding this comment

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

Understood and agreed about needing to make a copy of skymodel.flux. Good catch.

I think I understand and agree why we don't need to make a copy of the frame anymore and do the two-step sky-subtraction + cosmics mask + re-skysubtract, but I haven't reviewed that as closely.

I put a minor comment inline about ivar propagation, which pre-dates the fix of this PR itself. I don't think it matters, but please confirm. Most corrections are small enough to not matter, but I'm not sure if we every have large outliers where we would be making large (likely incorrect) changes to the flux without corresponding changes to the ivar that might be needed to meaningfully reject it downstream.

py/desispec/sky.py Show resolved Hide resolved
@akremin akremin merged commit 37cacfc into main Nov 3, 2022
@akremin akremin deleted the skysub-fix branch November 3, 2022 21:15
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

3 participants