updates and fixes to template-generating code and failing unit tests and errors

[moustakas]

[WIP]

In this mishmash PR I'm planning to address #507, #537, #549, and #558. I'll provide details of the changes when I'm done.

[moustakas]

Updates in this PR (still WIP, unit tests still not totally fixed):

• A long-standing issue with the current template-generating code in desisim.templates is that if a simulated template did not satisfy the target-specific color-cuts, then the code would issue a non-catastrophic warning and then return a spectrum of zeros. I think that's a defensible choice because it would be misleading to return a mix of spectra which do and do not pass the color-cuts; however, this choice has also caused a myriad of downstream problems (including Suspiciously low QSO flux #507 and suspiciously low ELG/LRG/QSO flux #549). So, to address this issue, here I've done two things:

  • I added an iterative element (with a default maximum of 10 iterations) to the GALAXY Class (used by the ELG, LRG, and BGS template-makers) to help ensure that a template (which satisfies the color-cuts) will always be generated.
  • I now raise an exception if a model cannot be generated, as proposed in desisim.templates should fail if a template that satisfies the colorcuts cannot be generated #537.

• We can revisit this issue if the changes above do not resolve the underlying issue, but I've generated hundreds of templates of each flavor and haven't seen a single failure yet. But note that the iterative piece does add a bit extra computing time, but I think it's worth it.

• The second long-standing issue I decided to fix was how we handle the velocity dispersion broadening, which made bitwise reproducibility hard to ensure. In this branch I've switched from using desisim.pixelsplines to scipy.ndimage.gaussian_filter1d. The upside is that the code is cleaner, faster, and reproducibility (to be demonstrated with some forthcoming unit tests) but the downside is that since I don't do any constant-velocity resampling of the basis templates, the velocity broadening is only approximately correct in the optical range. I don't think this shortcoming is a blocking factor, however.

• I'll document some additional fixes that are coming along in this PR next.

[moustakas]

@sbailey can I bring the trapezoidal rebinning script in redrock as a dependence to desisim (faster) or should I stick with desispec.interpolation.resample_flux (slower)?

[sbailey]

I'd prefer to not bring in a new desisim dependency on redrock, but if it really makes a substantial difference in pragmatic runtime, ok. If that dependency becomes problematic we can copy the code into desisim (or otherwise augment desispec.interpolation to have the faster simpler version from redrock; desispec.interpolation.resample_flux is slower because it does more with ivar weighting, masking, etc.).

[coveralls]

Coverage increased (+0.1%) to 45.64% when pulling a527625 on fix-failing-tests into 296bf7a on master.

[moustakas]

@weaverba137 would you be able to help me debug these failing tests? Everything passes on my laptop but I'm worried that I've messed up the test configuration somehow, e.g., import desimodel.fastfiberacceptance doesn't even work.

[moustakas]

Any thoughts on why the coverage tests fail and how I can fix this? Otherwise, this PR is ready for review (although I may have some additional small updates / bug fixes as I'm looking at the mock simulations in desitarget with this branch now, too.)

[weaverba137]

The test coverage has decreased dramatically! It's rare to see a PR that changes the coverage by that much. Did you add a lot of code that is not tested or did you disable tests of existing code?

[weaverba137]

Another discrepancy: You're using desimodel/0.17.0 code but desimodel/0.12.0 data. I've gone ahead and updated the CI test so that desimodel data is used consistently.

[moustakas]

Thanks. I added many more unit tests and not a lot of new code, so I would have expected the coverage to increase a lot, not the other way around.

[weaverba137]

OK, so we'll have to compare this branch to master.

[weaverba137]

If you look at https://coveralls.io/builds/45574713, coverage of desisim/templates.py has decreased by ~ 11%. That's a lot, and it accounts for the bulk of the overall decrease.

[weaverba137]

And test_templates.py is mostly commented out. So there isn't much of a mystery here. You must have thought you uncommented the tests, but didn't actually check in the change.

[weaverba137]

Still need to actually run tests on desisim/template.py. Other comments are relatively minor.

[weaverba137]

Coverage is back to pre-PR levels, even with a slight increase and all tests are passing, so merge when ready.