Add default option to precompute and cache legval values in _xypix

[lastephey]

This pull request includes a large re-arrangement in the structure of _xypix for gausshermite objects. There are now two options-- the default is to precompute the legval values for each patch and then look them up in _xypix, and the other is the original method of computing each legval value for each point in _xypix. This re-arrangement results in a new runtime of ~2:45 on Haswell as compared to ~4:15, and a similar fractional speedup on KNL.

There will be another corresponding pull request in desispec for the corresponding changes required in desi_extract_spectra to add these new options.

[sbailey]

I fixed the minor merge conflicts with master, but python setup.py test is still failing due to leftover no_cache options (perhaps you forgot to push your last set of changes?) Please update so that tests pass and then I'll re-review the final state.

[lastephey]

Yes, I have more changes to push. I'm in a workshop right now so I wanted to wait until it's over so it has my full attention.

On Thu, Aug 23, 2018 at 2:27 PM Stephen Bailey notifications@github.com wrote:

I fixed the minor merge conflicts with master, but python setup.py test is still failing due to leftover no_cache options (perhaps you forgot to push your last set of changes?) Please update so that tests pass and then I'll re-review the final state.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/desihub/specter/pull/70#issuecomment-415576673, or mute the thread https://github.com/notifications/unsubscribe-auth/AF8Y9GfQGLuSIhWgqDp6XpyIF9kUnBtSks5uTx5HgaJpZM4V8waG .

[sbailey]

That's fine. I'll wait for the final commits; I had mis-interpreted the readiness of the PR from a different email. Please add a comment to this PR when you are done making your changes and they are all committed and pushed.

[lastephey]

Sorry for the confusion, I definitely will.

On Thu, Aug 23, 2018 at 2:31 PM Stephen Bailey notifications@github.com wrote:

That's fine. I'll wait for the final commits; I had mis-interpreted the readiness of the PR from a different email. Please add a comment to this PR when you are done making your changes and they are all committed and pushed.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/desihub/specter/pull/70#issuecomment-415577671, or mute the thread https://github.com/notifications/unsubscribe-auth/AF8Y9FfUiA2ckYhuBKDwOFuXXUszZ_0Mks5uTx8fgaJpZM4V8waG .

[lastephey]

I believe all changes have been made and the Travis tests are passing. This is now ready for review.

I did verify that the output fits file is identical to the output of the pre-changes version of specter.

[sbailey]

Looks good; thanks. I agree that TraceSet.eval() has gotten a bit messy and could be cleaned up, but let's not block this PR for that. I removed a few leftover unused references to legval_dict in the non-gausshermite PSFs. After Travis tests re-pass I'll merge.

[lastephey]

Good find, thank you. Sounds good.

On Fri, Aug 24, 2018 at 1:53 PM Stephen Bailey notifications@github.com wrote:

Looks good; thanks. I agree that TraceSet.eval() has gotten a bit messy and could be cleaned up, but let's not block this PR for that. I removed a few leftover unused references to legval_dict in the non-gausshermite PSFs. After Travis tests re-pass I'll merge.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/desihub/specter/pull/70#issuecomment-415879591, or mute the thread https://github.com/notifications/unsubscribe-auth/AF8Y9MOI9skXYnQVj8FQLAzUY7aUIkR6ks5uUGfLgaJpZM4V8waG .