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

select calib dark with most cameras; fixes 20231028 for Jura #2253

Merged
merged 3 commits into from
May 16, 2024
Merged

Conversation

sbailey
Copy link
Contributor

@sbailey sbailey commented May 14, 2024

This PR fixes #2247 where the pipeline was generating a ccdcalib job for camword a01456789 but later needing CTE corrections for z3 not included in that camword.

The underlying reason was that the job camword is tied to the selected dark, and select_calibrations_to_proc was picking the first calib dark instead of the one with the most available cameras.

With this PR, desi_proc_night -n 20231028 --dry-run-level 1 now selects a different dark and generates a ccdcalib job for all cameras.

20231028 is the only night in Jura that I'm aware of that is impacted by this issue, though it is possible that other nights would pick a different dark with this branch.

Additional notes:

  • I pulled the dark selection out into a function select_calib_dark(etable). Upstream from that function the etable had already been filtered to 300 second darks. In general, I think we'd prefer a longer dark with more cameras over a 300 sec dark with fewer cameras, but I'm unaware of a night where that distinction matters so I didn't change that here. In the future select_valid_calib_exposures could be more permissive about what dark exposure times are allowable, and this new function select_calib_dark could include EXPTIME in its ranking consideration.
  • tying the ccdcalib camword to the dark camword is in principle more restrictive than it has to be, but I'm also unaware of a case where the best dark has fewer cameras than the set of zeros, so that distinction also may not matter (for now at least).

@sbailey sbailey requested a review from akremin May 14, 2024 23:02
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.

Thanks for the unit tests and the more robust dark selection. Given how the arc and flat selection works, I think we could have the function select_calib_dark return the desired etable row directly.

This will require a small rewrite, but I think it actually simplifies the function a bit. The unit tests will also need to be updated slightly.

@sbailey
Copy link
Contributor Author

sbailey commented May 16, 2024

Good idea. I updated select_calib_darks to return a Table instead of an index. Currently this is a Table of length 1, but in general in the future it could be a longer table to I made the function name plural and return a Table not a single Row object. This also simplifies its use with vstack with the other calib tables.

@sbailey
Copy link
Contributor Author

sbailey commented May 16, 2024

Design note: as originally implemented, select_calib_darks would raise a ValueError if no good darks were found. That breaks the updated test_missing_cals test when merged into main, so I updated this to return a length-0 table instead. Yay for unit tests; the updated test in PR #2254 just prevented us from accidentally re-breaking that code with this PR.

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.

Thank you for addressing my comment. Your changes look good. Merging now.

@akremin akremin merged commit f791b23 into main May 16, 2024
26 checks passed
@akremin akremin deleted the cte_camword branch May 16, 2024 21:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

20231028 camera z3 amplifier C: needs CTE correction or not?
2 participants