desihub / desisurvey

Code for desi survey planning and implementation
BSD 3-Clause "New" or "Revised" License
2 stars 7 forks source link

Progress to exposure #67

Closed weaverba137 closed 7 years ago

weaverba137 commented 7 years ago

This PR modifies desisurvey.progress.Progress.get_exposure() such that:

Please triple-check the logic for implementing PROGRAM. I wasn't quite sure if PROGRAM is something that needs to be a per-tile attribute or a per-exposure attribute.

sbailey commented 7 years ago

Thanks. PROGRAM is a per-tile attribute not a per-exposure attribute, so I fixed that logic. FYI, elsewhere in desitarget and desisim code, "OBSCONDITIONS" is the per-exposure concept of what the actual conditions are, regardless of the PROGRAM for which the tile was designed. Under normal operations the two remain in sync, but e.g. late in the survey if we run out of DARK tiles we may switch programs to observe BRIGHT tiles under DARK conditions.

I also reverted to not include EXPID by default, since those exposures don't include calibration exposures like darks and flats and downstream code would need to reassign those exposure IDs anyway to sync up with what gets simulated.

sbailey commented 7 years ago

Also for the record, I tested this at NERSC with $DESISURVEY_OUTPUT=/global/cscratch1/sd/dkirkby/desi/output/depth_0m and

from desisurvey.progress import Progress
p = Progress('progress.fits')
exp = p.get_exposures()
dkirkby commented 7 years ago

I had a quick look and don't see any problems. I won't get a chance for a more careful look until late next week, but I am happy going ahead on the PR before then.

One question: is PROGRAM necessary since it is duplicates info encoded in PASS? (and FITS string columns are a pain).

sbailey commented 7 years ago

PROGRAM is redundant information with PASS, but it is a pragmatic convenience choice to include it in the exposures table to enable more obvious and robust code like

if exp['PROGRAM'][i] == 'DARK':
    ...

instead of

if exp['PASS'][i] < 5:   # oops; dark is 0-3 not 1-4
    ...

Ideally we should also add desimodel.footprint.pass2program() and use that instead of hardcoding the mapping here, but that can come later.

weaverba137 commented 7 years ago

I just wanted to double-check something. If EXPID is not default, then

from desisurvey.progress import Progress
p = Progress('progress.fits')
exp = p.get_exposures()

won't include EXPID. Then the idea is that the exposure ID would be added when calibration exposures are merged with the science exposures, correct?

sbailey commented 7 years ago

Correct

If we really want to include EXPID by default here, we could also do something like add a gap of 100 "missing" entries in the sequence between each night, thus leaving plenty of room for calibration exposures to be assigned later. They real system can (and will) have gaps in the exposure ID sequence anyway, so downstream code should be able to handle non-contiguous exposure IDs.

weaverba137 commented 7 years ago

Sounds good, just needed the reminder.

dkirkby commented 7 years ago

They real system can (and will) have gaps in the exposure ID sequence anyway

What's the rationale for this? Doesn't that make them redundant with a timestamp? I was imagining the exposure ID living in a counter associated with the shutter firmware.

sbailey commented 7 years ago

When everything runs smoothly, there are no gaps in the EXPID sequence. In practice, e.g. from experience with SDSS and DES, occasionally the exposure ID counter will handout a new ID and then there is a hiccup such that it never makes it into any data. There will also be less pathological cases where an EXPID is used for actions like field acquisition or positioner iterations but never results in a spectrograph exposure.

i.e. ICS doesn't purposefully create gaps in the sequence, but downstream we should be robust to having gaps.