-
Notifications
You must be signed in to change notification settings - Fork 22
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
generate templates at given/input redshifts #132
Conversation
After many struggles with |
Odd. The last test run failed with:
even though the last commit was just for documentation and adding two additional test assertions in a different test, and the previous test run had passed. I've run |
I spent about an hour debugging this last night but it ended up being a problem in I don't know why the failure isn't reproducible but it's probably related to the fact that we're not fixing the random seed in I'm making 500 LRG spectra (using |
Comparing typical maxflux values, this test was too close to threshold. I loosened it. Merge conflict is due to doc/changes.rst . I will resolve that my hand and merge. |
Ah, I didn't see @moustakas comment prior to merge. If you can reproduce this, please open new PR fix based on a branch off of master. |
How's the timing with the new code? I haven't profiled it but it'd be great to know/check whether the bottleneck is still in the velocity dispersion convolution. I think @dkirkby had checked this previously. |
Note that the branch hasn't been deleted from GitHub. |
It takes about a second per spectrum, which is a bit problematic but not a deal breaker (14k CPU hours to simulate 50M DESI spectra...). As you noted separately, getting this correct was a good first step; making it faster would be helpful but can be treated as a separate issue. It would also be helpful to make the tests faster, which could be done just by checking multiple things per template set generation instead of re-generating templates multiple times just to check one thing each time. I'll delete the branch that Ben noticed was still there. |
This isn't ready to merge, but I'd appreciate eyes on some of the core code before I go and break/change everything else. The main point of this PR is to address #104, i.e., enabling templates to be generated at a given set of input redshifts, which we need for our full-survey data challenge. This turned out to be a harder problem than I had first anticipated, and required a complete rewrite of the Class for each spectral class; so far I've only rewritten the
desisim.templates.ELG()
Class. For example, there's some chunking and vectorized calls to thespeclite
filter functions as kindly suggested by @dkirkby to speed things up.If the user passes a redshift for which none of the templates pass the nominal color-cuts then a warning is issued and the output spectra and metadata table rows for those objects are simply returned empty; maybe we want this to be a more dramatic exception. I'm not sure, but @sbailey probably has an opinion.
However, in this situation it takes quite a long time (many 10s of seconds) to consider all ~8000 templates at the desired redshift, which is too long. For example, if you ask for 1000 templates at z=(0.1-0.4) which also pass the DESI ELG color-cuts then you better go for a long, long run instead of waiting around to be told that none of the templates were generated (i.e., not possible). I note that there's no clean, fast way of knowing ahead of time which templates will work out; you have to do the math (or pre-assign templates to redshifts, e.g., in the mocks, which is something we should pursue).
I plan on addressing this slowness issue by creating a more streamlined set of ELG basis templates. In the current set there's considerable overlap of the templates in color-color and physical parameter space, so we probably don't need more than a few hundred templates.
I still need to add a handful of unit tests, so stand by. And this PR also fixes #129 (for ELGs at least).