search

desihub / desispec

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

fix missing zall TSNR2 columns #2368

Closed sbailey closed 2 months ago

sbailey commented 2 months ago

This PR fixes the missing Kibo zall TSNR2_LYA and TSNR2_QSO columns reported in #2363 . This is required for making the final kibo/zcatalogs/v1/zall*.fits files.

The underlying issue is that desispec.zcatalog.update_table_columns makes some assumptions about column order of the input files while trying to standardize the output column order. The input order changed in Kibo (for reasons I haven't investigated) which broke those assumptions.

This is a minimal fix to get things to work for Kibo while still being backwards compatible with Iron etc. This update is only slightly less fragile than the original code, but while wrapping up Kibo I'm also trying to make the minimal working change and save deeper fixes for later.

The change: instead of assuming that TNSR2_LRG is the last TSNR2 column, check for for the last column that starts with TSNR2.

Test outputs using this branch are in $CFS/desi/spectro/redux/kibo/zcatalog/v1/zall--v1a.fits . These have the missing columns TSNR2_QSO and TSNR2_LYA at the intended location (after the other `TSNR2columns, and before the_NSPECand_PRIMARY` columns). Due to the size of the files and Perlmutter being out today, I have not checked that every row of every other column is unchanged, but I did check the first million rows and they exactly match as expected.

sbailey commented 2 months ago

I agree that this PR still has fragile assumptions, especially about the location of the NUMOBS_INIT and PLATE_RA columns relative to the *_TARGET and TSNR2_* columns. I don't think it is worth further generalizing the TSNR2 columns without also addressing those issues. I opened #2369 to track this after this PR is merged.

akremin commented 2 months ago

Thanks. We will address further robustness improvements in that PR. Merging this as good enough for now.