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

correctly select both "bright" and "faint" BGS templates by default #257

Merged
merged 7 commits into from
Sep 12, 2017

Conversation

moustakas
Copy link
Member

This PR fixes a bug identified by @akremin. Previously this snippet of code would fail to return any templates fainter than r=19.5:

from desisim.templates import BGS
flux, wave, meta = BGS().make_templates(nmodel=100, rmagrange=(19, 20))

because the BGS().colorcuts_function was being set to just desitarget.cuts.isBGS_bright, which applies the magnitude cut r<19.5 (note no equals sign).

In this PR I allow the option of the colorcuts_function attribute to be a tuple and the code checks whether any (using logical or) of the "color cuts" are satisfied. In the case of BGS these cuts are just magnitude cuts: r<19.5 for the bright sample and 19.5<=r<20 for the faint sample.

This bug does not affect the 2% sprint simulations because there the target selection cuts were applied outside of desisim.templates, but this is affecting @akremin's BGS-only bright-time simulations since the magnitude priors are being passed directly to desisim.templates via desisim.obs.new_exposure.

@moustakas
Copy link
Member Author

Tests are failing because the test_pixsim test is timing out; all the test_templates* tests are passing. I'd like to merge this but would like at least one other person to sign off. @weaverba137 @julienguy Anybody else?

@sbailey
Copy link
Contributor

sbailey commented Sep 11, 2017

Rerunning tests passed without problem; Travis was previously having a bad day (though we should watch out for whether we are on the edge for Travis timeouts).

Code changes look good. Thanks for fixing this while maintaining API backwards compatibility. I made one small change to enable passing in a list or tuple of functions and using isinstance. If/when the tests pass, ok to merge.

@sbailey
Copy link
Contributor

sbailey commented Sep 12, 2017

@weaverba137 could you take a look at the Travis sphinx build problem? Earlier tests in this PR used sphinx 1.5.6 and passed, but the latest update (not related to docstrings or .travis.yml) is now using sphinx 1.6.3 and failing. We've hit similar sphinx version problem before though I don't recall the solution.

@weaverba137
Copy link
Member

The errors are being caused by astropy. The .travis.yml file is forcing astropy 1.3.3, but this version is known to be incompatible with Sphinx 1.6. One option is to try the test with astropy 2.0. Another option would be to set the sphinx version in the CONDA_DEPENDENCIES variable.

@weaverba137
Copy link
Member

I have temporarily fixed this by downgrading to sphinx 1.5.

@sbailey sbailey merged commit c38dd4d into master Sep 12, 2017
@sbailey sbailey deleted the bgs-colorcuts-function branch September 12, 2017 17:41
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.

3 participants