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

Update quickgen and quickbrick to new specsim API #86

Merged
merged 9 commits into from
Feb 22, 2016
Merged

Conversation

dkirkby
Copy link
Member

@dkirkby dkirkby commented Feb 18, 2016

This is not ready to merge until I tag and release specsim v0.3, but now passes my tests on #80 using desihub/specsim#29. Please review if you are interested. I am finding identical results after the updates, but your mileage may vary.

Note that there are some trivial diffs in this PR where my editor (atom) automatically removed the invisible white space from the end of lines. Sorry about that. Could we standardize our whitespace conventions using editorconfig? For example, the conventions I use in specsim are specified here.

@sbailey
Copy link
Contributor

sbailey commented Feb 18, 2016

I was surprised by this snippet:

config = specsim.config.load_config('desi')
...
config.wavelength_grid.min = desimodel.io.load_throughput('b').wavemin
config.wavelength_grid.max = desimodel.io.load_throughput('z').wavemax

It seems that wavemin/wavemax from desimodel should be part of the default desi config. I haven't looked under the hood for how or where specsim is getting that desi config, however. Should that config be encoded as a file in desimodel itself? Perhaps desimodel should have a load_specsim_config() function that returns a properly configured specsim Configuration object for the parameters in that version of desimodel?

i.e. can we find a way to load a fully correct desi configuration directly, without having to load it and then modify it to get it into the actually correct state to use?

@dkirkby
Copy link
Member Author

dkirkby commented Feb 18, 2016

In this PR, I am aiming for the minimal changes required by the new specsim API, while preserving identical output results. In that spirit, I did not change how quickbrick sets its limits (which are different from those used by quickgen) even though it doesn't make any practical difference as long as the simulation limits include the throughput limits. However, it does make a tiny numerical difference since changing the grid min adds a sub-pixel offset to the grid, so I wanted to avoid that complication for my testing.

Looking ahead, it would be good to pick a canonical simulation range used by both quickgen and quickbrick, and update the specsim desi.yaml if necessary. Currently, it contains:

wavelength_grid:
    unit: Angstrom
    min: 3500.3
    max: 9999.7
    step: 0.1

While the throughput limits (which specsim also knows about from the throughput files) are:

>>> desimodel.io.load_throughput('b').wavemin, desimodel.io.load_throughput('z').wavemax
(3533.0, 9913.0000000092878)

Note that quickbrick originally used:

wavemin = desimodel.io.load_throughput('b').wavemin
wavemax = desimodel.io.load_throughput('z').wavemax
wave = np.arange(wavemin-1, wavemax+1, opts.wavestep)

but I eliminated the +/-1 padding in this PR since it makes no difference to the results.

@dkirkby
Copy link
Member Author

dkirkby commented Feb 20, 2016

I just tagged and released specsim v0.3 so this PR is ready to merge as far as I am concerned.

@sbailey
Copy link
Contributor

sbailey commented Feb 22, 2016

Looks fine to me. Refactoring the wavelength grid provenance can be a future PR. Merge away!

@dkirkby
Copy link
Member Author

dkirkby commented Feb 22, 2016

desihub/desimodel#7 is relevant to the question of what the canonical grid should actually be.

dkirkby added a commit that referenced this pull request Feb 22, 2016
Update quickgen and quickbrick to new specsim API
@dkirkby dkirkby merged commit 4ca150e into master Feb 22, 2016
@dkirkby dkirkby deleted the specsim_api branch February 22, 2016 14:52
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.

2 participants