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

Streamline use of specsim by quickgen and quickbrick #93

Merged
merged 17 commits into from
Mar 9, 2016

Conversation

dkirkby
Copy link
Member

@dkirkby dkirkby commented Mar 6, 2016

The changes here depend on specsim changes in desihub/specsim#35, which will be merged and tagged (as v0.4) before this PR is merged.

Both quickgen and quickbrick now have much cleaner and consistent interactions with specsim.

Externally visible changes to quickgen:

  • Add --seed option for reproducible results with different seeds (was always seed=0 before).
  • Same noise realization is used for frame and cframe files.
  • Noise and ivar are based on individual camera statistics, not the coadd, which is most noticeable at the camera edges.
  • Resolution is convolved with the output pixel size.
  • Sky model output now has non-zero ivar and noise (after fixing desihub/specsim/tie randseed to tileid; scale sky with airmass #1).

Externally visible changes to quickbrick:

  • Random numbers are now reproducible.
  • Remove --wavestep option (was 0.2A by default, now 0.1A from desi.yaml)
  • Remove --downsampling option (output pixels were 3 x 0.2A by default, now 0.5A from desi.yaml)
  • Resolution matrix now written correctly to brick files (was using non-standard number of off-diagonals).

Externally visible changes to specsim that affect both scripts:

  • The sky now has extinction applied.
  • Source flux can now disperse onto the CCDs from just outside.
  • Initializing a simulator no longer warns about non-standard FITS units.

@dkirkby
Copy link
Member Author

dkirkby commented Mar 6, 2016

Fixes #89

@sbailey
Copy link
Contributor

sbailey commented Mar 7, 2016

These changes all look good to me in both desisim and specsim, but it appears that our desisim travis tests pass without even installing specsim, so none of the quickbrick/quickgen changes are checked at all by the tests unless I'm missing something (i.e. my reading coverage of our travis config may be incomplete).

One question: are quickbrick/quickgen dependent upon desi.yaml in specsim, or is there a way to override which config file they use? In the long term we should re-consolidate on desimodel as being the only canonical description of the desi configuration (i.e. desimodel should include whatever config file format specsim needs); in the short-term it would be handy to provide an option to override which config file to use.

@dkirkby
Copy link
Member Author

dkirkby commented Mar 7, 2016

@sbailey This PR only updates the use of specsim by the quickgen and quickbrick scripts. I didn't touch the testing setup, but I will create a new desisim issue for this.

Yes, these scripts are dependent on desi.yaml in specsim but that hasn't changed with this PR. They can read this file from anywhere, so it would be straightforward to have them use a version under desimodel. I will create a desimodel issue for this.

in the short-term it would be handy to provide an option to override which config file to use.

Good idea. I will add that now.

@dkirkby
Copy link
Member Author

dkirkby commented Mar 7, 2016

I added an option --config to quickgen and quickbrick that defaults to desi (i.e., read desi.yaml from the data/config directory of the currently installed version of specsim), but also allows arbitrary yaml files to be used, e.g.

quickgen --config mydesi.yaml ...

A related feature that could be useful is to write a yaml output file that can be used to exactly reproduce a simulation job, i.e., including changes made at the command line (airmass, exposure time, ...) and using checksums to capture the versions of all external files (throughput, ...). I don't think we need robust provenance tracking and reproducibility right now, but its good to keep this functionality in mind so we don't make it unnecessarily hard to implement down the road.

@sbailey
Copy link
Contributor

sbailey commented Mar 8, 2016

OK to merge this whenever the specsim merge and tag is ready. Would be nice to include that in spectro pipeline + sims tags this week (maybe even today? tomorrow?).

@dkirkby
Copy link
Member Author

dkirkby commented Mar 8, 2016

Ok, I will try to finish this up today.

@dkirkby
Copy link
Member Author

dkirkby commented Mar 9, 2016

I just released specsim v0.4 and will now merge this branch.

dkirkby added a commit that referenced this pull request Mar 9, 2016
Streamline use of specsim by quickgen and quickbrick.  Fixes #89.
@dkirkby dkirkby merged commit cdb5063 into master Mar 9, 2016
@dkirkby dkirkby deleted the specsim_streamline branch March 9, 2016 04:20
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