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

updated all XGB code for n_estimators removal #123

Merged
merged 1 commit into from
Apr 3, 2016

Conversation

screwed99
Copy link
Contributor

What does this PR do?

Closing off the second half of #119 removing n_estimators from XGB operator.

Where should the reviewer start?

I basically copied change for change from your first merge on this at #122 except that there is no test coverage for theXGBClassifier class (it appears because it is not taken from the sklearn package list the other classes are.

How should this PR be tested?

I've run nosetests -s -c locally (Python 3 conda environment) and CI should be doing the rest.

Any background context you want to provide?

This is my first pull request ever so really any and all feedback thoroughly welcome!

Questions:

I'm interested in learning a bit more about your suggested workflow for doing pull requests:

  • The installation instructions for a user were in the documentation (and very clear) but I was wondering what conda environment you would suggest setting up for development (I would guess installing the tpot package is not necessary and for all testing of changes just set your working directory to inside the clones git project?)
  • Further to the above, the contribution guidelines mention CI but don't really mention how to test locally before committing, pushing and issues a pull request. I installed nose within my conda environment and ran nosetests -s -v as found in the CI files, is that sufficient?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.09%) to 28.932% when pulling e73ac08 on screwed99:xgb-operator-rework into 058cfea on rhiever:master.

@rhiever rhiever merged commit a27985f into EpistasisLab:master Apr 3, 2016
@rhiever
Copy link
Contributor

rhiever commented Apr 3, 2016

Many thanks, and nice work on your first PR. 👍

I was wondering what conda environment you would suggest setting up for development (I would guess installing the tpot package is not necessary and for all testing of changes just set your working directory to inside the clones git project?)

Hmmm... we should probably add section to the contributor document answering this question. The short answer is:

  • Use an install of the Anaconda Python distribution w/ Python 3, all packages updated to the latest version
  • pip install the other dependencies (DEAP, XGBoost, etc.)
  • Before making any changes to the code, create a new branch in your fork of the TPOT repo
  • cd into the base TPOT directory, and when running TPOT during development:
    • import TPOT normally if using the script version -- make sure you don't have TPOT installed locally, just in case, so you know that you're using the dev version
    • Call TPOT on the command line with python -m tpot.tpot

Further to the above, the contribution guidelines mention CI but don't really mention how to test locally before committing, pushing and issues a pull request. I installed nose within my conda environment and ran nosetests -s -v as found in the CI files, is that sufficient?

Yes, running nosetests in the base TPOT directory should run all the unit tests for you. We're still working on expanding the unit tests to cover more of the code base.

@screwed99
Copy link
Contributor Author

OK great, thank-you for the feedback!

Can I go ahead and raise an issue for the contributor document changes and submit a pull request with the changes (unless you'd prefer doing it another way)?

@rhiever
Copy link
Contributor

rhiever commented Apr 3, 2016

Yes, please feel free to do that. I will be happy to review the PR.

@stevenmaguire
Copy link

Congrats @screwed99!!!

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.

4 participants