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

refactored newexp #238

Closed
wants to merge 73 commits into from
Closed

refactored newexp #238

wants to merge 73 commits into from

Conversation

sbailey
Copy link
Contributor

@sbailey sbailey commented Apr 24, 2017

Disclaimer: this is a rather unsatisfying refactor since so much more could be done and it just doesn't "feel" right yet, but in the spirit of the 2% sprint I'm going to stop shuffling deck chairs and provide something that works for now.

Please review this but let's hold off on merging until we've confirmed that it properly handles the full 2% mock spectra input (I debugged it on a much earlier run). Tests pass on my laptop but I see I still have some Travis cleanup to do...

This refactors newexp-desi (now just newexp) to accept inputs from a mock target catalog, mock truth spectra, fiber assignment, and observing conditions from surveysim instead of making up a random exposure on the fly. It also switches to using specsim for the sky/lunar model and fiberloss calculations, also laying the groundwork for optionally using galsim for detailed per-object fiberloss calculations.

A random dark/bright/etc. exposure can still be generated from a python API (old desisim.obs.new_exposure code) but not via a command-line script anymore.

newarc and newflat are split out as separate scripts since they take different parameters than a new science exposure (e.g. no observing conditions or fiber assignment inputs...)

This also provides a simulate_spectra convenience function that wraps specsim to provide a translation from obsconditions and fibermap tables into how to construct and run a specsim Simulator. At its simplest this is just sim = simulate_spectra(wave, flux), but it optionally can take surveysim inputs for specifying observing conditions or a target/fiberassign/fibermap table to specify target locations and targeting bits to derive fiber input losses.

simspec format was simplified (?) to use binary tables with columns e.g. WAVELENGTH, FLUX, SKYFLUX instead of one image HDU per variable. If this mucks up BGS or other work that may have been writing simspec formats on their own, I could consider expanding desisim.io.read_simspec to support both formats.

Examples:

Generate new arc and flat exposures to be written to $DESI_SPECTRO_SIM/$PIXPROD/{night}/*-{expid}.fits:

newarc --night 20190830 --expid 1
newflat --night 20190830 --expid 3

Write output to a different location:

newarc --night 20190830 --expid 1 \
    --simspec temp/simspec-arc.fits --fibermap temp/fibermap-arc.fits

Generate a new science exposure using fiberassignment, surveysim, and mock spectra inputs:

newexp \
    --fiberassign fiberassign/tile_37299.fits \
    --obslist two_percent_DESI/twopct.ecsv \
    --mockdir mocktargets/ \
    --obsnum 0 --expid 4 --nspec 5000

This

  • reads the obsnum row from twopct.ecsv to get the exposure dateobs (and thus the YEARMMDD night) and observing conditions. That file does not have exposure IDs so that is separately assigned here (--expid 4)
  • reads fiberassign/tile_37299.fits to know what target was on which fiber
  • reads truth spectra for those targets from mocktargets/{abc}/truth-{brickname}.fits
  • puts it all together into simspec and fibermap files for output

The simspec+fiberassign outputs from these can be fed into either quickgen or pixsim, e.g.

simdir=$DESI_SPECTRO_SIM/$PIXPROD/20190830
pixsim --simspec $simdir/simspec-00000001.fits --nspec 100 --cameras b0,z0
quickgen --simspec $simdir/simspec-00000004.fits --frameonly

Future work that this PR does not try to address:

  • newexp runs a specsim simulation but it only needs the lunar model, throughput, and fiberloss model for output to specsim files. Quickgen then picks this up and runs the same specsim simulation again.
  • Update simspec to include the fibermap and any obsconditions metadata such that it becomes fully contained for describing a DESI exposure.
  • py/desisim/newexp.py became a bit of a dumping ground for new functions to try to keep them together and not break existing code. After we've sorted out what we really want, pieces could move elsewhere, e.g. reading mocks -> desitarget.
  • desisim.obs.new_exposure (previous code) generates a random exposure on-the-fly while desisim.newexp.newexp (new code) takes fibermap, mock, and observing conditions as input and generates an matching exposure. These are confusingly similarly names and could be consolidated.
  • better unit testing and and more robust/maintainable structure.
  • flux units. Either use astropy.units throughout, or standardize on 1e-17 erg/(s cm2 Angstrom) throughout. This PR tried avoid too many changes, but there is a bit of a delicate dance between things that require full units (specsim) vs. things that expect erg/(s cm2 Angstrom) vs. 1e-17 erg/(s cm2 Angstrom).
  • data format naming consistency. Minor inconsistencies between targets vs. fiberassign vs. fibermap led to some messy code.
  • arc inputs are still treated as a special case of electrons on the CCD instead of a surface brightness like the flat lamps.

@sbailey
Copy link
Contributor Author

sbailey commented Apr 26, 2017

Shoot. I completely forgot about random state seed propagation for this. Once we have the 2% inputs ready to use I'll test this with those anyway, but we should decide if reproducibility is a blocking factor or not for merging this (I'm inclined to think that it should be, but that may slow the sprint...)

@sbailey
Copy link
Contributor Author

sbailey commented Jul 26, 2017

heads up: I'm going to rebase this newexp branch to get the changes in #246 to get back in sync with API changes from desitarget/master and resolve merge conflicts with desisim/master. Usually rebasing after making a branch public is a Bad Idea, but I'm under the impression that I'm the only one developing on this branch.

@sbailey
Copy link
Contributor Author

sbailey commented Jul 28, 2017

The rebase was a massive mess, so I'm going to abandon this PR and delete the branch from github and re-push and start a new PR.

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