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 (again) #250

Merged
merged 60 commits into from
Aug 1, 2017
Merged

refactored newexp (again) #250

merged 60 commits into from
Aug 1, 2017

Conversation

sbailey
Copy link
Contributor

@sbailey sbailey commented Jul 28, 2017

This is the replacement PR for #238 that got bogged down in rebasing and merge conflicts due to keep the branch and PR open too long. Trying again.

Tests pass on cori; moving on to Travis now. Will also re-confirm with integration tests prior to final merge.

@moustakas
Copy link
Member

@sbailey Does this branch address #221?

@sbailey
Copy link
Contributor Author

sbailey commented Aug 1, 2017

This PR has gotten out of hand already. Will merge prior to implementing smaller isolated changes like #221 (Adding support for WD subtypes DA and DB). Running final (?) travis tests now, while checking simulating spectra tutorial in parallel.

@sbailey
Copy link
Contributor Author

sbailey commented Aug 1, 2017

Close notes:

This will require minor changes to the simulating spectra tutorial since the return values of desisim.obs.new_exposure changed, and the simspec HDU 'METADATA' changed name to 'TRUTH'. I'll submit that after this is merged.

I'll also submit an update to the desispec integration test to call newexp-random instead of newexp-desi.

quickgen runs and produces working cframe files, but the quicklook integration test fails on the sky subtraction step. It does work on master (after a minor desispec.pipeline.runcmd -> desispec.util.runcmd fix) so I broke something here, but I haven't been able to chase that down yet and don't want that to block this PR.

astropy.io.fits.BinTableHDU(meta) generates a broken HDU with our metadata tables from make_templates(). astropy.io.fits.convenience.table_to_hdu(Table(meta)) is the workaround. Unit tests didn't catch that problem, which is one symptom of this new code not having good test coverage.

A few features that I discussed with @moustakas and @akremin that didn't make it in here yet:

  • WD DA/DB
  • passing bgs_kwargs dict into obs.new_exposure() to propagate down through get_targets and eventually to BGS.make_templates()

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