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

new desi_group_spectra #505

Merged
merged 10 commits into from Feb 13, 2018
Merged

new desi_group_spectra #505

merged 10 commits into from Feb 13, 2018

Conversation

sbailey
Copy link
Contributor

@sbailey sbailey commented Feb 12, 2018

Opening this PR for review while I do the final cleanup (e.g. adding unit tests).

This PR is a complete rewrite of desi_group_spectra for several reasons:

  • propagates SCORES tables from cframe files into spectra files
  • 40x faster by using fitsio, optimized data appending, and smarter "do I really need to do this frame?" logic
  • isolates "which spectra files need to be updated" logic into a single function, which could be replaced in a future update with a spectro-processing DB query

Known issues I want to fix before merging:

  • no unit tests (I believe that was true of the previous version too)
  • --nights option became --night (easy to fix)

Known issues that may wait for a further update.

  • appending a new night that is completely missing one spectrograph channel for
    every frame, when the prior data did have that spectrograph channel.

In the name of optimization, this bypasses the standard Frame and Spectra objects and deispec.io.read/write routines. The FrameLite and SpectraLite objects here might be useful for other purposes (like QA) but are currently isolated into pixgroup.py and are not intended to be generally useful objects with the full features of Frame and Spectra. They are optimized for fast I/O and regrouping, but need unit tests to help ensure that they remain consistent with the standard more full-featured objects.

Copy link
Contributor

@julienguy julienguy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Pushed a modif to create output directory if missing
  • Tested on month simulation (5 nights, 827 spectra files). Ran successfully in 5min using 5 nodes on cori.

@sbailey
Copy link
Contributor Author

sbailey commented Feb 13, 2018

I changed the option back to --nights and added unit tests (and satisfyingly fixed a few bugs that were found...). Thanks for the check @julienguy. I'm ready to merge when travis tests pass.

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