desihub / specex

DESI spectrograph PSF fitting
BSD 3-Clause "New" or "Revised" License
0 stars 4 forks source link

Report failure when traces for good fibers overlap #70

Closed marcelo-alvarez closed 1 year ago

marcelo-alvarez commented 1 year ago

Modifies run_specex to return a non-zero value and print an error message to the log if the x (the direction perpendicular to wavelength on the ccd) coordinate of the trace for any given fiber not in the list of broken fibers passed to specex is not always greater than the preceding fiber and less than the following fiber. If the fiber is in the list of broken fibers, then only a warning is printed.

This indirectly addresses the issue in #69 since it would have prevented the erroneous traces from bad calibration data taken for 20220923 from being included in the default PSF that caused the subsequent arc failures.

I have tested this by processing the arcs from 20220923 with desi_run_night. Using specex from the main branch, processing for exposure 144026 completes successfully and outpus a PSF with unphysical overlapping trace fits, while on this branch the job fails and reports the bad fits in the log:

% grep overlapping /global/cfs/cdirs/desi/users/malvarez/spectro/redux/specex-offlamps/trace-overlap-20220923/run/scripts/night/20220923/arc-20220923-00144026-a0123456789-11560964.log
ERROR:qa.py:21:psf_qa: overlapping traces for fibers 486 and 487 in /global/cfs/cdirs/desi/users/malvarez/spectro/redux/specex-offlamps/trace-overlap-20220923/exposures/20220923/00144026/fit-psf-z7-00144026_19.fits
ERROR:qa.py:21:psf_qa: overlapping traces for fibers 486 and 487 in /global/cfs/cdirs/desi/users/malvarez/spectro/redux/specex-offlamps/trace-overlap-20220923/exposures/20220923/00144026/fit-psf-b7-00144026_19.fits
ERROR:qa.py:21:psf_qa: overlapping traces for fibers 487 and 488 in /global/cfs/cdirs/desi/users/malvarez/spectro/redux/specex-offlamps/trace-overlap-20220923/exposures/20220923/00144026/fit-psf-b7-00144026_19.fits
ERROR:qa.py:21:psf_qa: overlapping traces for fibers 170 and 171 in /global/cfs/cdirs/desi/users/malvarez/spectro/redux/specex-offlamps/trace-overlap-20220923/exposures/20220923/00144026/fit-psf-b4-00144026_06.fits
ERROR:qa.py:21:psf_qa: overlapping traces for fibers 276 and 277 in /global/cfs/cdirs/desi/users/malvarez/spectro/redux/specex-offlamps/trace-overlap-20220923/exposures/20220923/00144026/fit-psf-b4-00144026_11.fits

Issues that can cause these failures should result in flagging the entire exposure (as in the case for 20220923 where arc lamps were not being exposed correctly) and/or affected fibers as bad (due to being broken or having too low a throughput), and this new failure mode for specex provides an additional safeguard against unphysical calibration data being propagated downstream.

@sbailey @julienguy please have a look.

marcelo-alvarez commented 1 year ago

@julienguy thanks for the review. Having an overlapping trace QA function that takes file name and broken fiber list is a good idea and can be implemented easily in the way you suggest.

Not directly related to this PR: Another QA we are missing would be to make sure we address the correct fibers in the output of the trace shift fit. (I happened in rare cases, after a hardware change, that the PSF trace coordinate for fiber #n got centered on CCD trace #n+1 with an erroneous shift along X of several pixel). One could for instance identify the gaps between groups of 25 fibers and make sure they are addressed correctly.

Are the rare cases you mention of a shift in the addressing of the PSF trace coordinate after a hardware change related to what you reported in desispec #1555 , i.e. due to a bad column? @araichoor had a suggestion for finding the shift in desispec #1555 , so if this is the same (or closely related) issue then we could continue the discussion of this there.

marcelo-alvarez commented 1 year ago

@julienguy @sbailey I have modified the function psf_qa to have an explicit list of arguments (psf_filename, broken_fiber_list) and a doc string.

I tested that running on 20220923, for which the PSFs have multiple overlapping traces due to low transmission and arc illumination produces the error in this branch but not on main, e.g.:

cd  /global/cfs/cdirs/desi/users/malvarez/spectro/redux/specex-70
% tail -n 1 main-20220923/*/*/*/*/arc-20220923-00144025*.log
SUCCESS: done at Fri 21 Jul 2023 11:33:52 AM PDT
% tail -n 1 trace-overlap-20220923/*/*/*/*/arc-20220923-00144025*.log
FAILED: done at Fri 21 Jul 2023 11:28:45 AM PDT

I also spot checked that running on 20220922 (for which PSFs should have no overlapping traces) gives no errors for either branch, and also that all 150 output PSFs are unchanged between this branch and main, e.g.:

% for p in $(echo b r z); do for c in $(seq 0 9); do for exp in $(seq 899 903); do f1=main-20220922/exposures/20220922/00143$exp/fit-psf-$p$c-00143$exp.fits ; f2=trace-overlap-20220922/exposures/20220922/00143$exp/fit-psf-$p$c-00143$exp.fits ; fitsdiff $f1 $f2 | tail -n 1 ; done ; done ; done | grep "No differences found." | wc -l
150

It should be ready to merge after review.