Adding the new CTE correction to the pipeline

[julienguy]

In the PR, the CTE correction is included in the pipeline.

• New script desi_fit_cte_night and new I/O for CTE parameters

  • The script is to be run per night after the afternoon calibs in daily ops
  • It looks for LED 1s and 120s exposures in the exposure tables
  • preprocess them with a different output file (preproc-cte-CAM-NIGHT.fits)
  • Runs Eddie's model fit on the data for cameras with keywords CTECOLSX with X being an amplifier id (A,B,C,D)
  • writes output tables $DESI_SPECTRO_CALIB/$SPECPROD/calibnight/NIGHT/ctecorr-CAMERA-NIGHT.csv (or for each red and NIR camera by default)

• Changes to desi_proc

  • desi_proc -n NIGHT --nightlycte runs desi_fit_cte_night -n NIGHT in the pipeline
  • The batch version is not implemented (desi_proc -n NIGHT --nightlycte --batch crashes)

• Changes to desi_preproc

  • Cameras with keywords CTECOLSX have a new CTE correction applied at the preprocessing level
  • Uses row-by-row Horn extraction to get a precise image model
  • Uses the nightly CTE parameters

Requires modifications to the DESI_SPECTRO_CALIB yaml files. One can use the following version and try the code

export DESI_SPECTRO_CALIB=/global/cfs/cdirs/desi/users/jguy/teststand/desi_spectro_calib
export SPECPROD=XXXX
desi_fit_cte_night -n 20231111
desi_preproc -n 20231111 -e 204371 --camera z3

[schlafly]

One non-inline comment since I can't seem to comment on things that pre-dated this PR: desispec.correct_cte.correct_image code appears to require a cteparams input, and it is in the docstring, but it isn't explicitly in the function call signature so I don't see how it even runs and it appears that the function isn't used anymore. Has it been fully replaced by correct_image_via_model?

Thanks. This was my fault; the function should have had a cteparams input. As you suggest, the problem is that we don't actually use that mode, and instead use correct_image_via_model, and the two interfaces have fallen out of sync. We could just delete correct_image, though I think that approach is actually more correct in the limit that rowbyrow == spectroperfectionism. I did not go back to exploring that approach after implementing the model & rowbyrow components, though.

[julienguy]

I've posted some comments inline, many of which I have flagged for myself to implement, in particular adding MPI support for the pipeline.

One non-inline comment since I can't seem to comment on things that pre-dated this PR: desispec.correct_cte.correct_image code appears to require a cteparams input, and it is in the docstring, but it isn't explicitly in the function call signature so I don't see how it even runs and it appears that the function isn't used anymore. Has it been fully replaced by correct_image_via_model?

I removed the unused function desispec.correct_cte.correct_image

[sbailey]

As a heads up, I'm strongly considering changing this to output a single file per night instead of 20 files per night, where currently only 2 of them have actual corrections (z1,z3) and in much of the past none of them would have corrections. I think the simplification from fewer files (e.g. when combining files across nights to make trends) outweighs the under-the-hood complexity of gathering and stacking tables to write a single file. I will investigate that as part of adding MPI support for the pipeline (maybe that under-the-hood complexity is worse than I thought).

[sbailey]

Summary of my changes:

• added MPI support
• output one ctecorr file per night instead of per camera per night
• some semi-cosmetic rearrangement and docstring updates to help me understand the structure for long term maintainability
• minor bug fixes like creating output directory if it didn't already exist

The remaining piece is actually integrating with desi_proc_daily_manager and desi_run_night, which I'll need to discuss with @akremin . We currently start submitting jobs after the 300s DARK at the end of the ZEROs, before the ARCs and FLATs. Now we need to wait for the 1s FLAT before starting to process anything, but the current structure doesn't have hooks for "notice that we have received an ARC/FLAT that we need to process but wait until we get more data before submitting it because it will depend upon processing future data that may or may not eventually exist..."

Let's wait to merge this until we have a plan for that final integration, because merging this requires a 3-way coordination between desispec, $DESI_SPECTRO_CALIB, and nightwatch.

[sbailey]

Update:

• I've added desi_fit_cte_corr to the ccdcalib job, including a check to not run it if the ctecorr file already exists e.g. due to linking to a previous night.
• I've added a "wait_for_cals" step to the beginning of desi_daily_proc_manager to wait for the end of calibration files to arrive before starting any processing, which enables it to use the flats in the ccdcalib job (which was previously launched as soon as zeros+darks were done, before arcs+flats had arrived).

This is getting increasingly fragile, but enables us to run with the new CTE calibrations while waiting for a better pipeline cleanup from @akremin . I'll run in a separate side prod for a few nights to ensure that it really works before merging into main.

[sbailey]

My test run last night patiently waited until all of the calibrations had arrived before trying to process any of them, and then immediately fell on its face trying to CTE correct a dark image for identifying bad columns before the nightly ctecorr existed. I fixed that by updating desi_proc to not try to CTE correct ZEROs and DARKs.

@schlafly @julienguy if we get into a situation where the CTE corrections are large enough that we need to worry about their impact on dark current, we'll need to revisit this.

Before merging, I'm going to keep running the test prod mimicking daily ops until we have a night with actual on-sky science data to confirm that also gets CTE corrected correctly.

[schlafly]

That sounds right to me.

I hadn't thought about CTE in conjunction with darks and zeros before. I guess it's actually pretty important for hot pixels in both cases; since all the traps are empty those must get smeared badly. But we really don't have any mechanism to do the right thing there. The CTE modeling code uses a "model image" that would be ~all zero there, or something. The previous code that did not use a model image at the expense of correlating the read noise would conceptually do the right thing if somehow we already had the CTE parameters. But correlating the read noise is bad and would be most of what you end up doing on the darks and zeros.

I guess there's a world where when generating the master darks we have enough data that the read noise could be averaged down, and we have the past CTE parameters and could do something. But that's another project; I agree we shouldn't go there now!

[akremin]

In the ccdcalib job instead of running the cte detection after the bad column detection, we could run it before so that it is available for the dark.

If up-to-date bad columns are crucial we could have the cte detection create temporary versions of those files like it does the preproc of the flats.

The second portion of the suggestion would make a complicated procedure even more complicated, but I thought I'd throw it out there.

On Fri, Jan 26, 2024, 9:52 AM Eddie Schlafly @.***> wrote:

That sounds right to me.

I hadn't thought about CTE in conjunction with darks and zeros before. I guess it's actually pretty important for hot pixels in both cases; since all the traps are empty those must get smeared badly. But we really don't have any mechanism to do the right thing there. The CTE modeling code uses a "model image" that would be ~all zero there, or something. The previous code that did not use a model image at the expense of correlating the read noise would conceptually do the right thing if somehow we already had the CTE parameters. But correlating the read noise is bad and would be most of what you end up doing on the darks and zeros.

I guess there's a world where when generating the master darks we have enough data that the read noise could be averaged down, and we have the past CTE parameters and could do something. But that's another project; I agree we shouldn't go there now!

— Reply to this email directly, view it on GitHub https://github.com/desihub/desispec/pull/2163#issuecomment-1912455001, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACBM5O5KYUNEWZRBHIZQTFLYQPUOXAVCNFSM6AAAAABBXEVYUWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMJSGQ2TKMBQGE . You are receiving this because you were mentioned.Message ID: @.***>

[sbailey]

Yeah, all options are messy albeit in different ways. I'm going to leave it as-is for now unless we have a demonstrated case that it is worth the effort to change.

[sbailey]

@schlafly @SGontcho I'd like to put this into production on Thursday night, or otherwise wait until next week so that we're not making major changes over the weekend. This also requires a matching Nightwatch update from @sybenzvi to call preproc with --no-cte-corr. I updated that for qproc here (it will never try to ctecorr, because that isn't q=quick), but there is also a preproc-only mode for nightwatch that needs to be updated (same place in redrock.run.run_preproc that desihub/nightwatch#380 updated).

[sbailey]

There was a bug in preproc where --model-variance was replacing the cte-corrected image back to the original one (thanks to @julienguy for identifying that). I've fixed that, and verified that:

• for a non-CTE camera, this PR matches main with and without --model-variance
• for a CTE-corrected camera (z3), this PR
  • matches main with --no-cte-correction
  • meaningfully differs from main in the CTE-corrected region regardless of --model-variance or not
• --model-variance only changes ivar, but not the pixel image.

I'm re-launching night 20240128 in the /global/cfs/cdirs/desi/users/sjbailey/spectro/redux/ctetest production for testing...

[sbailey]

@schlafly @SGontcho @julienguy I've reprocessed /global/cfs/cdirs/desi/users/sjbailey/spectro/redux/ctetest night 20240128 (but not the other nights).

• The z3 preproc file now differ only in the CTE-corrected region (upper left).
• This leaks into the extracted raw frames at other wavelengths and fibers (lower left), which is perhaps a side-effect of differences in trace shifting the PSF before extraction? The scale is larger than the "divide-and-conquer" extraction sub-regions.
• This further propagates into the calibrated frames (cframe) differences (lower right), where the impact of both sky fibers and standard stars understandably impacts fibers other than just the CTE corrected ones.
• Redshifts are shown in upper right. There are ~15 differences, all coming from ZWARN>0 targets (red).
• I've verified that there are no differences on non-CTE corrected cameras, but I'd appreciate additional eyes on these results, especially the impact on non-CTE corrected fibers.

Plotting code is at /global/cfs/cdirs/desi/users/sjbailey/spectro/redux/ctetest/compare_exp.py

[schlafly]

Here's the tileqa comparison I usually make: https://data.desi.lbl.gov/desi/users/schlafly/misc/schlafly-20240128-ctetest-tileqa.pdf On this night I really only see an important effect on the redshifts in tileqa on 22161, the one you show, and it's pretty modest. But it is a definite improvement.

Note the patch of blue target redshifts around z ~ 1 in daily that disappear in ctetest.

I hadn't thought about impacts on PSF traces before, but I agree that we expect changes at more wavelengths after the sky subtraction, etc., starts correlating everything.

[sbailey]

Thanks for the QA check, @schlafly.

FWIW, I confirmed that the frame differences (before any sky subtraction or flux calibration that would correlate wavelengths and fibers) are almost entirely due to the difference in the PSF trace shifts. i.e. extracting the ctetest preproc image using the daily trace-shifted PSF results in frame differences mostly in the CTE-corrected region as expected:

There is a smattering of other isolated differences that I'm still not sure about, but it might be edge effects on the divide-and-conquer from how it was called in the pipeline vs. my isolated test.

I think we're good to go for putting this into production on Monday.

[schlafly]

I would also support putting it into production---thanks!

[sbailey]

I tagged DESI_SPECTRO_CALIB 0.5.1 as the last version before this change, then committed the updated spec/sm10/sm10-z.yaml (z1) and spec/sm6/sm6-z.yaml files (z3). Both DESI_SPECTRO_CALIB trunk and desispec/main have been updated at NERSC.

https://github.com/desihub/nightwatch/pull/390 is the matching PR from Nightwatch.