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

Progress to exposure #67

Merged
merged 8 commits into from
Oct 21, 2017
Merged

Progress to exposure #67

merged 8 commits into from
Oct 21, 2017

Conversation

weaverba137
Copy link
Member

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

  • All table columns are upper-case.
  • NIGHT is YYYYMMDD.
  • PROGRAM is included.
  • EXPID is included by default.

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.

@weaverba137 weaverba137 self-assigned this Oct 20, 2017
@sbailey
Copy link
Contributor

sbailey commented Oct 21, 2017

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
Copy link
Contributor

sbailey commented Oct 21, 2017

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
Copy link
Member

dkirkby commented Oct 21, 2017

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
Copy link
Contributor

sbailey commented Oct 21, 2017

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.

@sbailey sbailey merged commit b1def50 into master Oct 21, 2017
@sbailey sbailey deleted the progress2exposure branch October 21, 2017 15:05
@weaverba137
Copy link
Member Author

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
Copy link
Contributor

sbailey commented Oct 23, 2017

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
Copy link
Member Author

Sounds good, just needed the reminder.

@dkirkby
Copy link
Member

dkirkby commented Oct 25, 2017

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
Copy link
Contributor

sbailey commented Nov 2, 2017

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.

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.

3 participants