desihub / desitarget

DESI Targeting
BSD 3-Clause "New" or "Revised" License
18 stars 23 forks source link

Add check to run_mtl_loop that archived directories match the tiles-specstatus file #808

Closed geordie666 closed 10 months ago

geordie666 commented 11 months ago

This PR adds a check_archiving() function that is used in run_mtl_loop to test whether archived tile directories match the ARCHIVEDATE column in the tiles-spectstatus.ecsv file. The check is restricted to bright and dark tiles from the Main Survey.

See #807 and https://github.com/desihub/desisurveyops/issues/141 for more context.

run_mtl_loop now also includes a --noarchivecheck flag that can be used to bypass the archiving check (e.g., to speed up simulations or the alt-MTL process).

I tested this pretty extensively by:

The resulting error message was:

ERROR:mtl.py:494:check_archiving: archived tiles not in specstatus file: [3333]; tiles in specstatus file that aren't archived: [20994 25442]

@araichoor: I'd appreciate a second pair of eyes on this PR before I tag desitarget and we make the check a standard part of daily ops.

I don't think this is critical to resolve on the timescale of the long weekend, though!

coveralls commented 11 months ago

Coverage Status

coverage: 56.075% (-0.2%) from 56.25% when pulling 9bf035193de0df89703a5f5ca28ce24041facd6c on ADM-check-arxiv into 1e1034a35dbc5ab86ce33da484b4d792a7f6f5e0 on main.

geordie666 commented 10 months ago

Pinging @araichoor as a reminder. This still probably still isn't urgent yet, but it would be nice to merge this this week.

araichoor commented 10 months ago

sorry that fell off my radar. I ll have a look, and comment what I can.

geordie666 commented 10 months ago

Thanks for your careful review @araichoor. I addressed all of your comments. I went in the direction you suggested of splitting out the archiving check by main/bright and main/dark.

I've left one request for an opinion from @schlafly as part of my response to the final comment of your review. Otherwise I think this PR should be ready for merging.

I rechecked the code using the same test as detailed at the top of this thread.

araichoor commented 10 months ago

thanks for implementing my suggestions!

I just thought of a possible minor drawback of making the check on {survey,obscon} only, but I think there s nothing to change in the code.

I thought about the following:

geordie666 commented 10 months ago

I think "dark" and "bright" being "out-of-sync" is fine, though, right?

As far as I know we treat "bright" and "dark" as completely separate surveys? So, although we've never just run run_mtl_loop DARK or run_mtl_loop BRIGHT (because of the way the instructions are structured on the wiki) there's nothing, in principal, that's actually wrong with just running one or the other?

Sometimes we run an MTL update where there are, e.g., some new dark tiles but no new bright tiles and the run_mtl_loop BRIGHT command just does nothing in that case.

Another way of looking at this (which is why I liked your suggestion) is that we might want to proceed with an MTL update for dark tiles even if there's an archiving issue for bright tiles.

Am I missing something that intertwines the bright and dark parts of the survey?

araichoor commented 10 months ago

'I think "dark" and "bright" being "out-of-sync" is fine, though, right?' => yes, sure, no big deal actually

'Am I missing something that intertwines the bright and dark parts of the survey?' => no you re right!

I was just slightly worried that we end up in a case where run_mtl_loop has been run for one program but not for another one; but you re totally right that those are fully disjoint, and that this actually already happens if we do an mtl update but one program has no new tiles.

sorry for the noise!