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

ctecorr flexibility #2194

Merged
merged 5 commits into from Mar 18, 2024
Merged

ctecorr flexibility #2194

merged 5 commits into from Mar 18, 2024

Conversation

sbailey
Copy link
Contributor

@sbailey sbailey commented Mar 14, 2024

This PR makes the CTE correction code more flexible for cases when not correction is needed:

  • when generating the CTE corrections, desi_fit_cte_night first checks if a CTE correction is even needed for a camera before complaining about things like BADCAMWORD being set. It also checks the science exposures and doesn't try to generate a CTE correction for a camera that is flagged as bad all night anyway.
    • @akremin this still only checks CAMWORD and BADCAMWORD; I could use some help for how to also check BADAMPS
  • when using the CTE corrections, desi_preproc first checks if the correction is needed for a given camera before checking for the existence of $DESI_SPECTRO_REDUX/$SPECPROD/calibnight/NIGHT/ctecorr-NIGHT.csv . This restores the ability to preproc non-CTE-problematic cameras outside of the context of a production without having to specify --no-cte-corr. If DESI_SPECTRO_CALIB says that this camera on this night needs a CTE correction, then $DESI_SPECTRO_REDUX/$SPECPROD/calibnight/NIGHT/ctecorr-NIGHT.csv is still required unless the user opts-out with --no-cte-corr. @julienguy this should make you happier.

@sbailey sbailey requested a review from akremin March 14, 2024 17:37
@sbailey sbailey self-assigned this Mar 14, 2024
Copy link
Member

@akremin akremin left a comment

Choose a reason for hiding this comment

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

Overall this looks great. I have added some comments inline where I think one piece should be moved, and then I provide some additional functions that might be useful. I also address how to incorporate BADAMPS into this.

py/desispec/correct_cte.py Show resolved Hide resolved
py/desispec/correct_cte.py Outdated Show resolved Hide resolved
py/desispec/scripts/fit_cte_night.py Outdated Show resolved Hide resolved
py/desispec/correct_cte.py Outdated Show resolved Hide resolved
@sbailey
Copy link
Contributor Author

sbailey commented Mar 16, 2024

@akremin I believe I have updated the camword logic to cover the cases you raised, including BADAMPS via erow_to_goodcamword. Please re-doublecheck my logic.

Copy link
Member

@akremin akremin left a comment

Choose a reason for hiding this comment

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

All of my requests were addressed in the latest commits. The implementation at line 604 in get_cte_images() is better than my original suggestion because it quickly skips nights where the camera isn't used, while also capturing the case I was concerned about.

@akremin akremin merged commit c2222d2 into main Mar 18, 2024
26 checks passed
@akremin akremin deleted the cteflexibility branch March 18, 2024 22:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants