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

Regroup #473

Merged
merged 7 commits into from
Dec 11, 2017
Merged

Regroup #473

merged 7 commits into from
Dec 11, 2017

Conversation

sbailey
Copy link
Contributor

@sbailey sbailey commented Dec 1, 2017

This PR implements a number of updates to the spectral grouping, though it needs review and could benefit from some more updates before merging. Opening PR now so that this doesn't linger.

  • fixes bug when not using desi_group_spectra --pipeline option, which was causing it to read the same frame once per target instead of once per pixel and thus being very slow. That was the original bug I was chasing here and then it got out of hand with "just one more thing..."
  • The current pipeline planning identifies which exposures overlap each healpix, but doesn't distinguish which frames within those exposures do or do not overlap. This PR includes a pre-check of reading just the fibermaps to quickly discard any frames that don't actually overlap the healpix instead of committing to reading the full Spectra object with flux, ivar, resolution, etc.
    • this is done before distributing the tasks to ranks so that we don't accidentally end up with a bunch of ranks with nothing to do, but it would be better if this step was itself parallelized, re-grouped, and then re-distributed.
    • or even better, it could be done as a purely geometric calculation without having to read any input files.
  • it derives the Spectra fibermap format directly from the input cframe files fibermaps instead of hardcoding which columns are supposed to be there. This also enables it to to bulk copies instead of element-by-element copies, thus speeding up the Spectra creation by 10-20%.
  • It currently loses the header column comments in the fibermap HDU

It does not yet implement the final step of creating per-night slurm scripts for regrouping, nor does it fix the wavelength float -> double in frame, cframe, and spectra as suggested by @djschlegel and @tskisner .

There is also some commented out code that was useful for viewing while refactoring, but should be completely removed before merging.

This branch also still needs testing vs. master to verify that they actually do get the same spectra in both cases.

@sbailey sbailey requested a review from tskisner December 1, 2017 06:35
@sbailey
Copy link
Contributor Author

sbailey commented Dec 1, 2017

tests failing because spectra_columns() was removed. I'll fix tomorrow.

@tskisner
Copy link
Member

tskisner commented Dec 7, 2017

Thanks @sbailey , I'll try to make time to look through this in the next day.

@sbailey
Copy link
Contributor Author

sbailey commented Dec 7, 2017

Updates:

I converted wavelength to double instead of single precision in the frame, cframe, and spectra output files, since this was related to the updates in the Spectra class. Fixes #469.

In the process of re-adding comments to the fibermap HDU, that broke Bricks. These have been deprecated in favor of the Spectra objects/files, so I went ahead and removed them instead of fixing them. That led to removing the unused redmonster-centric zfind code which also used bricks. And then the super-optimized but slow and memory hungry coadds. I did leave bin/desi_inspect for inspecting data for a single target; it is broken without bricks, but has pieces we may want to reuse as we expand visualization tools.

@sbailey
Copy link
Contributor Author

sbailey commented Dec 11, 2017

I verified that this produces the same output (albeit with targets in a different order and extra columns) and that it runs faster than current master. We'll have more to do on this spectral regrouping topic, but I'm going to merge this now so that we can make use of the TILEID column that is needed downstream.

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