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

Update to LRG target selection code #179

Merged
merged 13 commits into from
Jun 27, 2017
Merged

Update to LRG target selection code #179

merged 13 commits into from
Jun 27, 2017

Conversation

geordie666
Copy link
Contributor

Moving the most recent Eisenstein/Dawson LRG code over from the sandbox so it's our default until the TS WG indicates otherwise.

@geordie666
Copy link
Contributor Author

Resulting LRG selection has about 590 targets per sq. deg. when run on DR3.1

@geordie666 geordie666 requested a review from moustakas May 24, 2017 16:42
@moustakas
Copy link
Member

Can you point me to the output files of running this new selection on DR3.1? I need to make sure the current set of LRG templates span the color-space of this new sample.

@geordie666
Copy link
Contributor Author

Sure...I'll send you that link over email.

@moustakas
Copy link
Member

With apologies for the delay, I'm looking at this now. @geordie666 could you fix the conflict(s) in the doc/changes.rst file?

Also note that we (someone not me!) will need to update the nominal redshift distribution in desimodel/data/targets/nz_lrg.dat with the one here.

@moustakas
Copy link
Member

@geordie666 I resolved the conflict in changes.rst and I added the following notebook for our own edification:
https://github.com/desihub/desitarget/blob/LRGupdatesADM/doc/nb/lrg-color-color.ipynb

I don't think that any shortcomings in the LRG templates (and there are some..) should hold up this PR, but these comparisons are good to see and will help inform the next set of template updates.

I'll also share the notebook on-list for input and comments.

@moustakas
Copy link
Member

@geordie666 Can you or Kyle update the n(z) for this selection?
#179 (comment)

Are we ready to merge this? I'm working on updated templates but what I'm using are good enough for now.

@geordie666
Copy link
Contributor Author

geordie666 commented Jun 20, 2017 via email

@geordie666 geordie666 merged commit 86b0b64 into master Jun 27, 2017
@geordie666 geordie666 deleted the LRGupdatesADM branch June 27, 2017 21:02
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.

2 participants