Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removing bad exp for darks and biases #1570

Merged
merged 47 commits into from Jan 11, 2022

Conversation

Waelthus
Copy link
Contributor

@Waelthus Waelthus commented Jan 5, 2022

This PR is supposed to tackle #1511 and #1562.
I.e. for both the standalone make_dark_scripts/desi_compute_dark_nonlinear as well as the nightly desi_compute_nightly_bias it incorporates checking of BADAMPS/BADCAMWORD/CAMWORD.
For both types of analysis exposures will only be used when processing a given camera when it is not part of BADAMPS/BADCAMWORDS and either not be part of the exposure_table at all or part of CAMWORD and has LASTSTEP='all'.

Exposures are treated in the following fashion:
If no exposure_table entry exists, we assume all is fine with them.
Else, every exposure with LASTSTEP!=all will be ignored for all cams (this was used for nightlybias before, but not for standalone). Cameras that are flagged inside BADAMPS or BADCAMWORD as well as cameras that are not inside CAMWORD will not be used either.

For the nightly: if there are enough exposures, i.e. by default nzeros=25, with all cams working, only those are used. Only when there are less, exposures with problems on some cameras will still be used for the others.
In cases where an exposure does not even reach minzeros anymore for some camera (e.g. r1 on 20201217 where all exp are bad), all other cameras will still be processed and an error is written to the log (instead of raising an Exception). Please tell if this is the behaviour we want or if we should fail completely (maybe for simpler bookkeeping?).

For the standalone: Will always use whatever is available without being flagged. This required a modification of the speclog produced by make_dark_scripts to track which cams are good/bad.

I tested this for the nights of 20211203 (some exposures with BAD z3, but second set was taken, for both nightly and standalone BAD z3 exposures are correctly ignored, and depending on the number of zeros requested for the other spectrograph the first set is still used) and 20201217 (BADAMP r1A leads to discarding all r1 spectra for nightly_bias [with SPECPROD="f4"] no output file is produced for this, other cams are still processed as normal).

Copy link
Member

@akremin akremin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you Michael! Overall this looks great and I think you made the right decisions in what zeros to keep in each scenario. It looks like it will work as-is, but I have some requested changes. The requests are to increase clarity and optimize a little bit. There are a lot of things, but they should all be simple changes. Please let me know if you need clarification on anything.

Since some of my requested changes move lines around, I'll wait to test your code until after you address the requests.

If I could make one final request: to speed up testing could you provide the commands you used in your debugging and/or point us to a directory at NERSC with outputs of your tests?

bin/desi_compute_dark_nonlinear Outdated Show resolved Hide resolved
bin/desi_compute_dark_nonlinear Outdated Show resolved Hide resolved
bin/desi_compute_dark_nonlinear Outdated Show resolved Hide resolved
bin/desi_compute_dark_nonlinear Show resolved Hide resolved
py/desispec/ccdcalib.py Outdated Show resolved Hide resolved
py/desispec/ccdcalib.py Show resolved Hide resolved
py/desispec/ccdcalib.py Outdated Show resolved Hide resolved
py/desispec/ccdcalib.py Show resolved Hide resolved
py/desispec/ccdcalib.py Outdated Show resolved Hide resolved
py/desispec/ccdcalib.py Outdated Show resolved Hide resolved
@Waelthus
Copy link
Contributor Author

Waelthus commented Jan 7, 2022

thanks @akremin. I modified the code according to your suggestions.

The tests can be found on cori: /global/cscratch1/sd/mwalther/desi/PRs/desispec/1570/.
For testing I used SPECPROD=f4, such that exposure tables are found which mark things as bad.

  • Subdirectory testing_darks:

    • steps:
      • run create.py (which calls make_dark_scripts)
      • modify output slurm script to load development env
      • run slurm script
    • this runs on 20211203 and 20211204 and reports 88 ZEROs and 36 DARKs for b2, but 63 ZEROs and 36 DARKs for z3 on 20211203 (i.e. the latter skips 25 bad ZEROs, both skip the bad DARK), for the other date both use the same number of exp.
  • Subdirectory, testing_nightly_bias:

    • tests on 20201217 (to check if the behaviour for missing zeros is as expected when BADAMPS was set and when no replacements where taken)
      • steps: submit nightlybias.sl
        this correctly treats r1 which has no valid exposures and is skipped, all other cameras are reduced, "1/30 failed" is reported.
    • tests on 20211203 (to check if the partials work as expected based on BADCAMWORD)
      • steps: submit nightlybias_alt_64.sl
        set desired number of zeros to a high number, but for z3 only 25 exp will be usable and the logic for partial obs is needed. This uses 48 exp for all but z3 and 25 for z3 (2 of the 25 bad zeros where dropped for all as they were the first 2 of the night).
      • steps: submit nightlybias_alt_25.sl
        same as above but requesting to use 25 only, as that amount is found for all cams, the partials are ignored.
      • In both cases all 30 bias frames are generated.

Copy link
Member

@akremin akremin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for addressing the changes and the quick turn around. Everything here looks great.

I tested the bias code on two additional nights:

  1. 20210503 with BADCAMWORD="r6z8" set for a subset of zeros
  2. 20211109 with BADAMPS="b5B;b5D" for all zeros.

The logs indicate that it did the right thing in each case and the output files were also as expected. In case (1) the two cameras were missing for biases (leaving behind biasnighttest's instead). In case (2) the one camera was missing for biases, but that is because the dark was also missing and the submitted cameras didn't include b5. I.e. that is a feature rather than a bug, but worth noting for the future.

I have not done an independent test of the dark model code, but will trust Michael's tests. That is not part of the normal processing, so we can patch any subtleties with that later on in the unlikely event that we need to.

@akremin akremin merged commit 1083566 into desihub:master Jan 11, 2022
@Waelthus Waelthus deleted the removing_bad_exp_for_darks branch January 11, 2022 08:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants