search

desihub / desispec

DESI spectral pipeline
BSD 3-Clause "New" or "Revised" License
33 stars 24 forks source link

cctecorr code doesn't support links to a ctecorr file from another night #2197

Closed akremin closed 3 months ago

akremin commented 4 months ago

ctecorr-{NIGHT}.csv includes a NIGHT column that appears to be checked by the code and raises a critical error if it differs from the night of the data the code is trying to process. This means that cctecorr doesn't currently support the linking of calibrations from a different night.

sbailey commented 4 months ago

Ugh! The motivation for including the NIGHT column was so that we could stack N>>1 different ctecorr-NIGHT.csv files and get a table to track corrections vs. time.

  1. I think the solution here is to relax the check on NIGHT to be fairly flexible, like +/- 2 weeks to the current night in the hopes that we'd never have to span an outage longer than that.

Added to Jura To Do list.

sbailey commented 3 months ago

@akremin for consideration, it might be a better solution for linkcal to copy the contents of the reference ctecorr file, replace any NIGHT entries with the new night, and write it back out for the new night. This would allow code using the file to be agnostic about how the file got there and just use it, without requiring parsing which night in the file is closest to the desired night.

Ideally we would include a comment that this night had been replace, but unfortunately vanilla csv doesn't formally support comments. They could be read with Table.read(filename, comment='#') but that's a potential user-interface landmine, and writing it with a comment would require re-opening to add the comment. Given that we have ~50 ctecorr files in daily already, I don't think we should change the format to ecsv or yaml or add a COMMENTS column though. Or we could e.g. define a new COMMENTS column and retroactively add them to all daily ctecorr files. Or just live without having a comment about what night this came from.

akremin commented 3 months ago

I'm not sure that it's better to alter the information in the file rather than just adding a few lines of code to interpret whether the night where the correction was taken is compatible with the current night of processing. Especially given that there is no metadata propagation, as you point out, I think it would be better to keep the night column as the true night the values were estimated from. This is in the same line as not changing the NIGHT header keyword in the other linked cals, so that we know where they actually came from (in addition to the links which also indicate where they're coming from).

sbailey commented 3 months ago

OK, thanks for considering the options. Fine to proceed with updating the user/reader side.

akremin commented 3 months ago

Resolved with PR #2213