desihub / desispec

DESI spectral pipeline
BSD 3-Clause "New" or "Revised" License
36 stars 24 forks source link

Allow different CTE functions for different nights / cameras. #2221

Closed schlafly closed 5 months ago

schlafly commented 5 months ago

This is a draft pull request so that Stephen and Julien can see what I'm thinking for incorporating different CTE transfer functions.

The approach is:

There are related changes:

I have more work to do running this on a number of different nights, etc., but if this is the wrong organization I should change that ASAP.

sbailey commented 5 months ago

Documenting zoom chat with @schlafly:

Basic concept seems good. Given that you are changing the format of the ctecorr files anyway (different column names), consider switching to a yaml format like:

20240215:
    z3C:
      columns: '1668:2057'
      function: simplified_regnault
      params: {amplitude=120.2703, fracleak=0.2275}
      chi2pdf: 2.0897

This would preserve the ability to stack ctecorr files, while allowing significant flexibility for what params format goes with which function with code like

# ctecorr = ctecorr_from_yaml[night][camera_amp]
func = get_transfer_function(ctecorr['function'])
result = func(pixval, in_trap, **ctecorr['params'])
...

OK to debug with what structure that you currently have to test algorithmic performance, but let's reconsider this formatting before putting into production.

schlafly commented 5 months ago

I finally got the nightqa to run and Anand signed off on this; we could put this in. I have not implemented an alternative CTE file format. I can do that, but I would want to rerun a couple of nights to make sure that everything is okay, so if perlmutter remains unhappy that might take a while.

When this PR or a close relative is merged we will also need to merge updates to DESI_SPECTRO_CALIB to use it. $DESI_SPECTRO_REDUX/schlafly3/desi_spectro_calib has those changes, but they have not been merged.

schlafly commented 5 months ago

This PR now includes changes to use a yaml format instead of the original format. The format is not quite the same as Stephen proposed above, and is instead: [schlafly@login06 desispec]$ cat ~/desiproject/spectro/redux/schlafly3/calibnight/20240310/ctecorr-20240310.yaml

- ALPHA: 1.0054078480743123
  AMPLIFIER: D
  BETA: 0.9182803966209395
  CAMERA: r7
  CHI2PDF: 5.054348278024068
  CMAX: 55.79834949505703
  FIN: 1.1891513415754933
  FOUT: 0.23380825656008195
  FUNC: regnault
  NIGHT: 20240310
  SECTOR: 2056:3406
- ALPHA: 1.2443239682117555
  AMPLIFIER: B
  BETA: 0.8335822096378048
  CAMERA: z7
  CHI2PDF: 1.7016646152794195
  CMAX: 62.57042974311667
  FIN: 1.7003719200642955
  FOUT: 0.2472963906647175
  FUNC: regnault
  NIGHT: 20240310
  SECTOR: 2057:3724

These are still stackable and can have multiple entries per amplifier, though in practice we can't use that feature yet. It's worse in that I haven't broken out "parameters" separately, though, and I haven't introduced a new layer of dictionary-ing under night.

In discussions about how best to test this Stephen points out that desi_fit_cte_night ends up getting called with the specific exposures it needs rather than having it find the best exposures. We need to update that list to include the 3 and 10 s flat fields, when available. I don't know where that code lives.

I generated a number of nightly CTE calibration files as part of testing this PR. That showed that z7 is weird---it got a lot worse from May to November 2023, and hasn't changed much since then. OTOH, r7 has gotten steadily worse at ~1 electron / day since February or so, which is a larger slope than we usually see but is otherwise fairly typical. I don't have any special prescription to deal with that so am not proposing we do anything there, but it's noteworthy.

I am running another set of full nights now with an updated DESI_SPECTRO_CALIB to try to judiciously set simplified_regnault vs. regnault depending on the amplitude of the CTE effect.

sbailey commented 5 months ago

The CTE exposure selection in the pipeline is a combination of

both of those overlap with desispec.correct_cte.get_cte_images which is what is called if desi_fit_cte_night doesn't get explicit EXPIDs.

@schlafly are there any real constraints to which exposures are passed forward, or do you just need at least two exposures of different exposure times? e.g. if we take flats in both the afternoon and the morning, is it a problem to get both? Do we need to filter down to sets of flats taken close in time, and/or with unique EXPTIME?

@akremin or I can make the workflow update for what expids are bassed to desi_fit_cte_night, but I'd like to understand what the real requirements are for which flats to select. If it isn't trivial to describe, we can discuss on the data call on Tuesday and then document here for the record.

akremin commented 5 months ago

As Stephen mentioned it is easy to either (A) provide all cte exposures, or (B) select one of each exptime. But if we need the cte exposures to be a consecutive set acquired together then that will take slightly more coding (though still straight forward).

schlafly commented 5 months ago

The comparison is across the amplifier rather than among the various exposures. I think the "most understandable" would be the four adjacent 1, 3, 10, 120 s flat fields with the same lights, but it doesn't have to be that and can just be any good 1, 3, 10, 120 s flats. I wouldn't send more than one of each flat. i.e., if there were a lot of different flats of each of those times, in principle the code could handle that, but it would be easier to understand what's happening if we always just send the same 1, 3, 10, 120 set for each night's cals.

sbailey commented 5 months ago

Looks good! I confirmed with @akremin 's updates that multiple CTE flat expids are passed forward if available, and re-verified that the chain of ccdcalib -> ctecorr file -> preproc works. I am trusting @araichoor and @schlafly 's analysis that the improved fits are indeed better.

I will merge this PR now. @schlafly please commit your changes to desi_spectro_calib and confirm when that is done.

schlafly commented 5 months ago

@sbailey , I have updated desi_spectro_calib. I note that my changes conflicted with changes to update the z7 pixmask, so I had to fix that up. I think I did that correctly, but I haven't rerun nights to verify...

sbailey commented 5 months ago

Noted, thanks. We'll include some recent nights with new z7 CTE and new z7 pixmasks as part of Jura pre-flight test runs to check that.