desihub / desispec

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

review desi_compute_sky defaults #2005

Open sbailey opened 1 year ago

sbailey commented 1 year ago

The pipeline calls to desi_compute_sky include a lot of non-default options, e.g.

desi_compute_sky \
    -i /dvs_ro/cfs/cdirs/desi/spectro/redux/iron/exposures/20201215/00067972/frame-r0-00067972.fits.gz \
    --fiberflat /dvs_ro/cfs/cdirs/desi/spectro/redux/iron/exposures/20201215/00067972/fiberflatexp-r0-00067972.fits.gz \
    --adjust-wavelength --adjust-lsf \
    --pca-corr /global/cfs/cdirs/desi/spectro/desi_spectro_calib/0.4.0/spec/sm4/skycorr-pca-sm4-r0.fits \
    --fit-offsets --skygradpca /global/cfs/cdirs/desi/spectro/desi_spectro_calib/0.4.0/spec/sm4/skygradpca-sm4-r0.fits \
    --tpcorrparam /global/cfs/cdirs/desi/spectro/desi_spectro_calib/0.4.0/spec/sm4/tpcorrparam-sm4-r0.fits \
    -o $SCRATCH/sky.fits

It's fine for the pipeline to be very explicit about its inputs, but in the spirit of "the defaults should do the recommended right thing", review what would happen if the call was just data inputs and outputs and not calibrations and options:

desi_compute_sky \
    -i /dvs_ro/cfs/cdirs/desi/spectro/redux/iron/exposures/20201215/00067972/frame-r0-00067972.fits.gz \
    --fiberflat /dvs_ro/cfs/cdirs/desi/spectro/redux/iron/exposures/20201215/00067972/fiberflatexp-r0-00067972.fits.gz \
    -o $SCRATCH/sky.fits

I suggest that the desi_spectro_calib inputs should be looked up automatically if they aren't specified, with an option to turn them off if needed. Similarly --adjust-wavelength --adjust-lsf --tpcorrparam could use parser syntax like

parser.add_argument('--adjust-wavelength', action=argparse.BooleanOptionalAction, default=True)

to retain the current syntax while making that option True by default and also supporting --no-adjust-wavelength if/when you need to turn that off.

This occurred to me because of PR #2001 where @segasai et al are experimenting with alternate sky fibers, and they may be unaware (until now!) of all of the non-default options currently used by the pipeline for standard processing.

segasai commented 1 year ago

Thanks for pointing at that @sbailey. I had indeed a command with a few options from somewhere, but I wasn't sure I had included all the options needed there. I guess I'll use the command you've given above instead for the time being.

segasai commented 1 year ago

a connected issue when trying to run the sky script (using suggested options above) I am not sure I get sensible results. I.e. this script

desi_compute_sky -i /global/cfs/cdirs/desi/spectro/redux/daily/exposures//20221130/00155832//frame-b8-00155832.fits --fiberflat /global/cfs/cdirs/desi/spectro/redux/daily/exposures//20221130/00155832//fiberflatexp-b8-00155832.fits --adjust-wavelength --adjust-lsf --pca-corr /global/cfs/cdirs/desi/spectro/desi_spectro_calib/0.4.0/spec/sm4/skycorr-pca-sm4-b0.fits --fit-offsets --skygradpca /global/cfs/cdirs/desi/spectro/desi_spectro_calib/0.4.0/spec/sm4/skygradpca-sm4-b0.fits --tpcorrparam /global/cfs/cdirs/desi/spectro/desi_spectro_calib/0.4.0/spec/sm4/tpcorrparam-sm4-b0.fits -o /global/cscratch1/sd/koposov/m31_newsky//00155832/sky-b8-00155832.fits

makes the sky which does look corrupted comparing to say /global/cfs/cdirs/desi/spectro/redux/daily/exposures/20221130/00155832/sky-b8-00155832.fits.gz

A lot of messages say 'WARNING:sky.py:1178:calculate_throughput_corrections: cannot corr. throughput. for fiber ...'

Any suggestions what could/(or what I'm doing) wrong here ?

thanks

segasai commented 1 year ago

Scratch my last comment. I didn't really understand what sm1, sm2 were, and that they are a (nontrivial) function of the petal number.

sbailey commented 1 year ago

Indeed, smX is the hardware identifier, which has a ~random mapping to which petal that hardware is plugged into. For the record, the mapping is in desi_spectro_calib/0.4.0/spec/smsp.txt:

sp0 sm4
sp1 sm10
sp2 sm5
sp3 sm6
sp4 sm1
sp5 sm9
sp6 sm7
sp7 sm8
sp8 sm2
sp9 sm3

This confusion is another good example for "defaults should do the recommended right thing" so that the end user running desi_comput_sky wouldn't have to look up the mapping in the first place because it would auto-select the correct --pca-corr etc calib files.