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

poststd can't fit more cams than stdstar did #1916

Merged
merged 3 commits into from Nov 22, 2022
Merged

Conversation

sbailey
Copy link
Contributor

@sbailey sbailey commented Nov 22, 2022

This PR fixes a bug discovered on 20221121, where z7 was bad but b7 and r7 were still good. The stdstar jobs were excluding sp7, but the poststdstar jobs were still trying to include b7 and r7 and failing since stdstar-7-*.fits didn't exist. It's unclear to me whether this is a new bug, or whether in the past we flagged all cameras of a spectrograph as bad if any of them were bad since we knew we couldn't get to redshifts for that spectrograph anyway (I have some vague memory of that hack...)

This PR fixes it by checking the PROCCAMWORD of each poststdstar job and making sure that it doesn't exceed the cameras included in its corresponding stdstar job.

I have not tested whether we have a similar bug in use-tilenight, but since that follows a different logic flow it may not. This is being tested in-situ on the 20221119 daily processing using this branch, and so far so good...

Note: this PR is into the "daily" branch, and then we'll merge that back into main.

@sbailey sbailey added this to In progress in dailyops via automation Nov 22, 2022
@sbailey
Copy link
Contributor Author

sbailey commented Nov 22, 2022

commentary: at first I thought the fix would be to ensure that poststdstar jobs were only using complete spectrographs like stdstar jobs, but that isn't correct either, e.g.

TILEID  EXPID  CAMWORD
100     123456  a0123
100     123457  a012b3r3

In this case expid 123457 could still flux calibrate b3 and r3 since a3 was included in another exposure for that tile. But if both exposures had been missing z3, then a3 would dropped from stdstar fitting and b3 and r3 couldn't be flux calibrated in a poststdstar step.

Before merging, let's also test this with --use-tilenight on perlmutter, but I'm stopping for tonight. 20221119 jobs have made it through poststdstar and are now running redshifts. Let's finalize and merge this before launching 20221120.

@sbailey
Copy link
Contributor Author

sbailey commented Nov 22, 2022

I added a fix for tilenight too, and tested it on an edited case from 20221119:

EXPID  TILEID CAMWORD BADCAMWORD  -> PRECAM      STDCAM  POSTCAM
153847 83205  a0679   z7             a069b7r7    a069    a069
153848 83205  a0679   z07            a69b07r07   a069    a69b0r0

where PRECAM, STDCAM, and POSTCAM are the camwords it is supposed to use for prestdstar, stdstar, and poststdstar steps. e.g. POSTCAM doesn't include a7 because z7 was bad for both exposures, but it does include b0r0 on the second exposure because z0 was available on the first one and thus could be used for stdstar fitting.

I verified that they are the camwords used by tilenight when calling desi_proc and desi_proc_joint_fit (tested by actually running), and that these are the camwords used by non-tilenight in the slurm jobs created (tested with processing 20221119 last night, and they re-tested with latest code with --dry-run-level 1 and inspecting scripts).

@marcelo-alvarez marcelo-alvarez merged commit a637160 into daily Nov 22, 2022
dailyops automation moved this from In progress to Done Nov 22, 2022
@marcelo-alvarez marcelo-alvarez deleted the poststd_cameras branch November 22, 2022 20:07
@marcelo-alvarez
Copy link
Contributor

Thanks, this looks good to me, merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants