Add UPDATED timestamp column

[weaverba137]

This PR:

• Updates desispec.tilecompleteness.merge_tile_completeness_table() to add an UPDATED column for database use.
• Closes Filter or remove NaN warnings when reading GFA summary files #2367. Warnings now will only be printed if the corresponding exposure table entry is not already set to zero.
• When changes to GFA entries are detected, the night is recorded, which modifies the input to compute_tile_completeness_table. In one scenario, delayed GFA processing can result in updates to the tiles file, even if a night was not included in --nights $NIGHT.
• Adds some cosmetic and documentation changes to desi_tsnr_afterburner.

Open questions:

• There was an open question about how to return the output of one_night() to main() when running in MPI mode. Is desi_tsnr_afterburner ever run in MPI mode?
• Why are args.expids and args.nights parsed more than once?

Some operational testing should be performed before merging this PR.

[coveralls]

coverage: 30.164%. remained the same
when pulling feb45e1 on add-updated-column
into 97b1748 on main.

[sbailey]

There was an open question about how to return the output of one_night() to main() when running in MPI mode. Is desi_tsnr_afterburner ever run in MPI mode?

We have used desi_tsnr_afterburner in MPI mode in the past, but that didn't scale up to current production sizes due to limitations of how the MPI path was implemented (needing to recalculate things that aren't recalculated in the multiprocessing path). For Kibo we used multiprocessing with one job per night, then combined them afterward. Daily mode also uses multiprocessing, not MPI. i.e. I don't think we are actively using the MPI option now (but as part of a future refactor, I would like to re-enable an MPI option with better scaling).

Why are args.expids and args.nights parsed more than once?

Smells like a leftover from iterative code development. i.e. I don't see a specific reason for that (e.g. a modification of the originally parsed list of nights/expids, requiring an re-parsing of the original args).

[sbailey]

Conceptually this seems fine, though I haven't done any operational testing of it myself. @akremin could you help with that to make sure it works as expected for daily ops?

@weaverba137 does this require the patched exposures and tiles files to be in place with the pre-added UPDATED column before this can be used, or will it add that UPDATED column to the current files if we merge this before signing off on the patched files? i.e. what is the order of operations for deployment (which might also be relevant for @akremin testing)?

[weaverba137]

does this require the patched exposures and tiles files to be in place with the pre-added UPDATED column?

That would be my preference. It would increase code complexity if we had to assume UPDATED may or may not be present in both the previous data and the new tiles.

[akremin]

Thanks Ben, the changes look good. I agree with the above comments that we'll want to coordinate things such that we put the updated tables in place before we try to run this with the merged code.

One question I have is whether we also want to have this column in the exposures file? Maybe I misread the code, but it seems like the UPDATED is only added for the tiles completeness that feeds into the tiles file.

[weaverba137]

When the daily database is updated, that update is tile-based, not exposure based, so we actually need UPDATED in tiles-daily.csv, not in exposures-daily.(fits|csv).

That brings up another open question. As I mention in #2335, desi_tsnr_afterburner and its relatives should write both fits and csv. Is there a specific, operational reason why we don't write a tiles-daily.fits file?

[akremin]

Okay, if we don't need it, that is fine. We can always add a column in the future if we decided we like having it.

There is no major reason why we don't write a tiles-daily.fits. Some (myself included) prefer csv's because they are easily parsable with e.g. grep and awk. exposures-daily.fits was necessary because of the additional HDU's that are included in the fits version of the file that can't be included naturally in a csv. Since tiles-daily.* only contains a single table there was no reason why we needed an additional fits version. However, for the productions we do produce a fits version for symmetry with the exposures file, and I would be open to a similar argument in daily.

On a more practical side, the tiles code only writes one format at a time, so we have to run the code twice to produce the files for e.g. Kibo. However, I'm sure that would be an easy modification to the code to make it write both simultaneously.

[weaverba137]

On a more practical side, the tiles code only writes one format at a time, so we have to run the code twice to produce the files for e.g. Kibo.

Right, I mentioned that in #2335, and yes it would be easy to fix

But since you mention it, there's actually a "feature" in desi_tsnr_afterburner such that if you pass the path to the tiles file with extension .csv, it will write the csv. However, if you pass it with extension .fits, it will write both. I don't think that should be a "feature" and these should just always write both.

[akremin]

I am fine with having the code always write both. I agree that it should either always write both or it should only write what is requested. The behavior or writing both under one circumstance but only one under another circumstance should be removed.

[weaverba137]

I've modified the code so that both files are always written out.

[weaverba137]

@sbailey @akremin, in preparation for (hopefully) merging this soon, I think we agreed to punt these open questions to a future refactor of desi_tsnr_afterburner:

There was an open question about how to return the output of one_night() to main() when running in MPI mode. Is desi_tsnr_afterburner ever run in MPI mode?
Why are args.expids and args.nights parsed more than once?

So I think the only thing left is to run operational tests after patching the daily tiles and exposures files.

[akremin]

With the latest push the code now works and I see updates for the night I tested.

Test:
I copied the latest files in daily to a local directory, checked out this branch, and ran the following:
desi_tsnr_afterburner -o exposures-daily_2.fits --prod daily --tile-completeness ./tiles-daily.csv --aux /global/cfs/cdirs/desi/survey/observations/SV1/sv1-tiles.fits --gfa-proc-dir /global/cfs/cdirs/desi/survey/GFA/ --compute-skymags --add-badexp --update --nproc 32 --nights 20240928

The updated table had the same "UPDATED" date for all nights except those on the night requested, which were updated to the time I ran it.

Two entries from 20240928 in the new file and the pre-updated file. They are different time stamps, which is good:

kremin@perlmutter:login04:/global/cfs/cdirs/desi/users/kremin/workspace/add-updated-column> grep 20240928 tiles-daily.csv /global/cfs/cdirs/desi/spectro/redux/daily/tiles-daily.csv 
tiles-daily.csv:22258,main,bright,bright,mainbright,1,773.0,40.505,-9.963,29.2,35.5,0.0,180.0,obsstart,32.7,39.9,35.5,16.1,37.808,0.85,20240928,2024-10-07T14:35:55-0700
tiles-daily.csv:11839,main,dark,dark,maindark,2,2385.7,48.809,-13.555,1006.5,1053.4,0.0,1000.0,obsend,1053.4,1042.0,1081.7,1065.0,485.86,0.85,20240928,2024-10-07T14:35:55-0700
/global/cfs/cdirs/desi/spectro/redux/daily/tiles-daily.csv:9332,main,dark,dark,maindark,2,1976.3,344.928,-10.544,1006.5,1068.2,1093.5,1000.0,obsend,1068.2,1077.8,1099.4,1191.6,dark,0.85,20240928,2024-10-07T12:03:11-0700
/global/cfs/cdirs/desi/spectro/redux/daily/tiles-daily.csv:9345,main,dark,dark,maindark,2,2240.7,339.44,-12.966,1008.9,1058.9,1160.4,1000.0,obsend,1058.9,1071.8,1086.8,1098.6,dark,0.85,20240928,2024-10-07T12:03:11-0700

Two entries from 20240927 in the new file and the pre-updated file. They are the same, which is good:

tiles-daily.csv:27154,main,bright,bright,mainbright,2,2169.2,331.757,-13.178,181.9,245.3,284.7,180.0,obsend,228.3,219.4,245.3,149.8,bright,0.85,20240927,2024-10-07T12:03:11-0700
tiles-daily.csv:6733,main,dark,dark,maindark,1,1464.0,28.484,18.944,1008.3,1042.7,1181.1,1000.0,obsend,1042.7,1046.1,1043.0,1527.8,dark,0.85,20240927,2024-10-07T12:03:11-0700
/global/cfs/cdirs/desi/spectro/redux/daily/tiles-daily.csv:27154,main,bright,bright,mainbright,2,2169.2,331.757,-13.178,181.9,245.3,284.7,180.0,obsend,228.3,219.4,245.3,149.8,bright,0.85,20240927,2024-10-07T12:03:11-0700
/global/cfs/cdirs/desi/spectro/redux/daily/tiles-daily.csv:1565,main,dark,dark,maindark,2,2625.2,32.936,19.265,1002.5,1112.4,1239.9,1000.0,obsend,1112.4,1160.5,1095.3,969.7,dark,0.85,20240927,2024-10-07T12:03:11-0700