-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
[MRG + 1] Add conventions section to userguide. #4508 #4566
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
Conversation
Thanks, this looks good.
a little git side-note: please don't merge master into your branch. if you want to update your branch,
I hope we add that to the docs in #3912. Thanks :) |
btw, have you tried building the docs and a) does it look good b) does sphinx throw any errors / warnings [related to this part]? |
I pointed out that classification targets are preserved. I used Thanks for you hint to use |
Thanks, looks good to me. |
>>> np.random.seed(0) | ||
>>> X = np.random.rand(100, 10) | ||
>>> y = np.random.binomial(1, 0.5, 100) | ||
>>> XX = np.random.rand(5, 10) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use a PRNG instance to make the execution deterministic.
>>> rng = np.random.RandomState(0)
>>> X = rng.rand(100, 10)
>>> y = rng.binomial(1, 0.5, 100)
>>> XX = rng.rand(5, 10)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe also rename XX
to X_new
or X_test
to be more explicit.
shrinking=True, tol=0.001, verbose=False) | ||
|
||
>>> clf.predict(iris.data[:3]) # doctest: +NORMALIZE_WHITESPACE | ||
array(['setosa', 'setosa', 'setosa'], dtype='<U10') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is causing the test to fail on old versions of Python. To avoid that we could do:
>>> predicted = clf.predict(iris.data[:3])
>>> list(predicted)
['setosa', 'setosa', 'setosa']
@cangermueller also please try to avoid merging master into a feature / bugfix branch as it makes it hard to rebase and squash intermediate commits later. Ideally it would be great to start branch again off of current master, |
If you are new to squashing stuff, copy your local repo to make a backup. |
Travis is failing because of Python2/Python3 string encoding types. Use +ELLIPSES |
I incorporated your comments. However, I had to create a new branch to rebase master (#4594). The next time I know how to do it properly ;-). |
Basically for the reasons Olivier mentioned above, you don't want any merge commits, i.e. commits with messages like "Merge ...", in your PR branch. Note that your alternative PR still has the same problem. Something that should work: # rename your current feature branch in case something goes wrong
git branch -m 4508 4508_bak
# update local master
git checkout master
git pull upstream master
# Create a new feature branch and cherry-pick meaningful commits
# Note that 6df4eac has already been merged into master
git checkout -b 4508
git cherry-pick 6035c98
git cherry-pick 5831524
git cherry-pick 234be23
# force-push into your PR branch
git push origin 4508 -f If the "Merge ..." commits disappear from the PR and you win ! You should double-check whether all the changes you wanted to make are still there to make sure that you didn't forget to cherry-pick any commits. |
pushed as 0fe613e |
I think you need to
|
Thanks @cangermueller and @amueller ! |
Not that this is really important, but |
Adds conventions section with two subsections about a) type casting and b) updating model parameters to quick-start userguide. See #4508.