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

Add notebook for RF QSO targets selection #664

Merged
merged 19 commits into from Dec 22, 2020

Conversation

echaussidon
Copy link
Contributor

Hi,

I write a jupyter notebook explain how the RF for qso targets selection were generated.

I also put the scripts for the training in : desitarget/train

Files are saved in Nersc at : /global/cfs/cdirs/desi/target/analysis/RF

@echaussidon
Copy link
Contributor Author

For the review : @geordie666

Copy link
Contributor

@geordie666 geordie666 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@echaussidon: Thanks for your extensive effort! I didn't have time to try the full tutorial, but the initial parts that I did try worked well.

Documentation tests are failing because the api.rst file that tracks documentation for Read The Docs needs to be update to reflect your new py/desitarget/train/ directory structure.

In addition to making the changes I've highlighted in the review, you'll also need to update the api.rst file in your branch to add the modules in each of your __init.py__ files.

Also, in the api.rst file, make sure to remove the reference to .. automodule:: desitarget.train.train_mva_decals which you are removing as a directory in py/desitarget/train/.

Once the documentation (sphinx) tests pass, I'm happy to merge this.

py/desitarget/train/__init__.py Outdated Show resolved Hide resolved
py/desitarget/train/data_collection/__init__.py Outdated Show resolved Hide resolved
py/desitarget/train/data_preparation/__init__.py Outdated Show resolved Hide resolved
py/desitarget/train/train_test_RF/__init__.py Outdated Show resolved Hide resolved
py/desitarget/train/train_test_RF/util/__init__.py Outdated Show resolved Hide resolved
@echaussidon
Copy link
Contributor Author

@geordie666 : I think every thing works now ! I'm not very familiar with api.rst, ect ...

Thanks for your review.

@geordie666
Copy link
Contributor

Thanks @echaussidon. And, thanks also for fixing-up all of the code style. I usually address that in my "spare time", but it's definitely good practice to adhere to PEP 8 style. I'll merge this soon.

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.

None yet

2 participants