Darks 2025 #2495
Darks 2025 #2495
Conversation
|
This PR is ready for review. Example run: Most of the time is spent on preprocessing. If we think we would frequently recompute the darks, we could saved the preprocessed exposures. Note that the preprocessing is done without masking or dark correction (of course), so the files should have a different name or be saved in a different directory than the other ones. |
…dir. raise error if matching biasnight does not exist
| @@ -1,36 +1,159 @@ | |||
| #!/usr/bin/env python | |||
There was a problem hiding this comment.
Please move this script into a callable function in desispec.scripts so that the pipeline can call it with MPI parallelism.
| sys.exit(1) | ||
| if args.nights is None and args.image is None : | ||
| log.error("Need to specify input using either the --nights or --image argument") | ||
| sys.exit(1) |
There was a problem hiding this comment.
When moved into a script in desispec.scripts, these sys.exit(1) should be replaced with return 1 and let the outer wrapper script in bin do something like sys.exit(desispec.scripts.dark.main()). i.e. we don't want the function itself to call sys.exit since that makes it harder to wrap in a pipeline.
| parser.add_argument('-i','--image', type = str, default = None, required = False, nargs="*", | ||
| help = 'path of raws image fits files (or use --nights)') | ||
| parser.add_argument('-n','--nights', type=str, default = None, required=False, | ||
| help='YEARMMDD nights to use (coma separated or with : to define a range)') | ||
| parser.add_argument('-o','--outfile', type = str, default = None, required = True, | ||
| help = 'output median image filename') | ||
| parser.add_argument('--camera',type = str, required = True, |
There was a problem hiding this comment.
Let's also support -c as shorthand for --camera
| file, then use that bias for all darks. If it is a list, it must have | ||
| len(rawfiles) and gives the per-file bias to use. | ||
| Note: if bias is None, the bias will be looked for in | ||
| $DESI_SPECTRO_REDUX/$SPECPROD/calibnight then $DESI_SPECTRO_CALIB |
There was a problem hiding this comment.
I don't like this default, since in most cases that is the wrong bias to use and will result in bad preprocessing for the darks. I would prefer the default to crash if it can't find the nightly biases it needs, with an option to opt-in to using a potentially dangerously bad bias as a fallback. At minimum there should be an option to opt-out of using that fallback so that a missing nightly bias doesn't just trigger a log warning and move on.
| max_temperature_diff (float) : maximal CCD temperature difference | ||
| reference_header (dict) : reference header that defines the valid hardware configuration (default is the most recent one) | ||
| save_preproc (bool) : save preprocessed images | ||
| output_specprod_dir (str) : specify output specprod directory used to save preproc images |
There was a problem hiding this comment.
Commentary: this doesn't actually have to be a real specprod. It just has to be a directory, under which the preproc files will be saved in dark_preproc/NIGHT/EXPID/ . And since this directory is also used as input if those files exist, the "output" part of the name is a misnomer. I don't immediately have a better name suggestion, though please clarify the help string to mention that it will read from there too if they exist.
| @@ -239,6 +239,7 @@ def findfile(filetype, night=None, expid=None, camera=None, | |||
| fibermap = '{specprod_dir}/preproc/{night}/{expid:08d}/fibermap-{expid:08d}.fits', | |||
| preproc = '{specprod_dir}/preproc/{night}/{expid:08d}/preproc-{camera}-{expid:08d}.fits{compsuffix}', | |||
| preproc_for_cte = '{specprod_dir}/preproc/{night}/{expid:08d}/ctepreproc-{camera}-{expid:08d}.fits{compsuffix}', | |||
| preproc_for_dark = '{specprod_dir}/dark_preproc/{night}/{expid:08d}/preproc-{camera}-{expid:08d}.fits{compsuffix}', | |||
There was a problem hiding this comment.
Good that these are kept in a different directory tree since they are different from normal preproc images (dark hasn't been applied). In addition to changing the directory name, let's also change the filename prefix (preprocdark?) to follow the principle that we shouldn't have two files with the same name in two different directories (i.e. the filename alone is enough to disambiguate)
| including enumerations of ranges given. | ||
|
|
||
| Note: this follows python-style ranges, i,e, 1:5 or 1..5 returns 1, 2, 3, 4 | ||
| unless `include_end` is True, which then returns 1,2,3,4,5 |
There was a problem hiding this comment.
At first I was going to object that this duplicates parse_int_args, but then I saw that you are using parse_int_args and then dropping entries that aren't valid dates, which is nice to handling ranges that span month boundaries. Please update docstring to mention that, since these examples actually don't apply to this function (1:5 would drop all entries).
For the record: I checked and this handles leap-years correctly :)
| parser.add_argument('-i','--image', type = str, default = None, required = False, nargs="*", | ||
| help = 'path of raws image fits files (or use --nights)') | ||
| parser.add_argument('-n','--nights', type=str, default = None, required=False, | ||
| help='YEARMMDD nights to use (coma separated or with : to define a range)') |
There was a problem hiding this comment.
When the date range spans a hardware change, how does this determine which configuration to keep vs. reject? It seems like we need another option to specify the night for which we are creating the dark (and potentially how to disambiguate if the config changed in the middle of that).
| if k=="MJD-OBS" : | ||
| table["MJD"]=tmp_table[k] | ||
| else : | ||
| table[k]=tmp_table[k] |
There was a problem hiding this comment.
When using the exposure_tables, we should also filter on LASTSTEP, CAMWORD, BADCAMWORD, and BADAMPS so that if we have flagged a dark as bad it won't be picked up here. @akremin could advise on utility functions to simplify this (I think we have something that takes a row of a table and tells you which cameras are ok to use (possibly none), or something like that).
This PR contains several updates to the script that computes a master dark (simple scaling with exposure time).