desihub / desispec

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

fix incorrect desi_fit_stdstars pipeline logic #158

Closed sbailey closed 8 years ago

sbailey commented 8 years ago

The current desispec.pipeline logic incorrectly treats desi_fit_stdstars.py as working on an individual frame, like all of the other calibration steps. However, desi_fit_stdstars.py simultaneously fits all 3 cameras of a spectrograph so that it finds the best template for each standard star. It only needs to be run once per spectrograph, not once per frame. Inspecting test output from the current pipeline shows that stdstars-[brz]{n}-{expid}.fits are identical.

The pipeline logic is being refactored, but this throws a new wrinkle in the dependency tree since desi_fit_stdstars.py currently depends upon 10 input files over 3 frames:

i.e. desi_fit_stdstars.py needs to wait for all 3 different frames to finish with sky subtraction before it can proceed. I think the currently pipeline may accidentally enforce that by at least running all sky steps before moving on to the stdstar steps.

desi_fit_stdstars.py should also gracefully fall back to using 2 channels if a 3rd channel is missing. At first I thought that that the pipeline should check those inputs and not bother running unless all 3 exist, but that creates a fragility if one CCD fails. It might be better for the pipeline not to enforce that, but just require 2/3 to exist and then proceed, letting desi_fit_stdstars.py itself handle a missing channel.

tskisner commented 8 years ago

Looking at this now as part of the refactor. @sbailey, your text above mentions wanting to "gracefully recover" if some of the inputs to a pipeline step do not exist. This goes against our pipeline planning philosophy so far. We don't currently have the concept of "optional" dependencies. Either they are a dependency or not. If one frame fails sky subtraction, then it seems like that is an error that should be fixed before proceeding with later steps. Currently the pipeline runs each macroscopic "step" in sequence, so all sky files have completed or failed before the stdstar step.

tskisner commented 8 years ago

Another point here is that currently in the data model, there is one stdstars-band-expid.fits file per exposure. Since there is one file generated per spectrograph, it seems like it should be named stdstars-spec-expid.fits. Or else we have to write out the one file and copy it 3 places... I guess I'll change the name and then update the data model when I do the final merge of this work into master.

tskisner commented 8 years ago

sorry- I meant there is one stdstars file per camera, rather than one per spectrograph. I'll change that.