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

[MRG + 1] Add conventions section to userguide. #4508 #4566

Closed
wants to merge 7 commits into from
Closed

[MRG + 1] Add conventions section to userguide. #4508 #4566

wants to merge 7 commits into from

Conversation

cangermueller
Copy link
Contributor

Adds conventions section with two subsections about a) type casting and b) updating model parameters to quick-start userguide. See #4508.

@amueller
Copy link
Member

Thanks, this looks good.
One addition I'd like to include is that classifiers can take arbitrary objects as labels and will reproduce them.
so you can do

SVC().fit(iris.data, iris.target_names[iris.targets])
and the predictions will be the string target names.

a little git side-note: please don't merge master into your branch. if you want to update your branch,

git checkout master
git pull upstream master
git checkout feature-branch
git rebase master

I hope we add that to the docs in #3912.

Thanks :)

@amueller
Copy link
Member

btw, have you tried building the docs and a) does it look good b) does sphinx throw any errors / warnings [related to this part]?

@cangermueller
Copy link
Contributor Author

I pointed out that classification targets are preserved. I used doctest to test the documentation and checked if everythinks looks nice.

Thanks for you hint to use rebase instead of merge!

@amueller
Copy link
Member

Thanks, looks good to me.

@amueller amueller changed the title Add conventions section to userguide. #4508 [MRG + 1] Add conventions section to userguide. #4508 Apr 10, 2015
>>> np.random.seed(0)
>>> X = np.random.rand(100, 10)
>>> y = np.random.binomial(1, 0.5, 100)
>>> XX = np.random.rand(5, 10)
Copy link
Member

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)

Copy link
Member

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')
Copy link
Member

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']

@ogrisel
Copy link
Member

ogrisel commented Apr 13, 2015

@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, git cherry-pick the commits that are not merge commit from this branch, squash them all to a single commit and then push -f the result of that operation to cangermueller 4508 to update this PR.

@ogrisel
Copy link
Member

ogrisel commented Apr 13, 2015

If you are new to squashing stuff, copy your local repo to make a backup.

@amueller
Copy link
Member

Travis is failing because of Python2/Python3 string encoding types. Use +ELLIPSES

@cangermueller
Copy link
Contributor Author

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 ;-).

@lesteve
Copy link
Member

lesteve commented Apr 15, 2015

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.

@amueller
Copy link
Member

pushed as 0fe613e

@amueller amueller closed this Apr 15, 2015
@ogrisel
Copy link
Member

ogrisel commented Apr 15, 2015

I think you need to git branch -D 4508 just before the git checkout -b:

# 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

# Delete the '4508' branch: the code there will still be available under
# the branch 4508_bak if you need it.
git branch -D 4508

# 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

@ogrisel
Copy link
Member

ogrisel commented Apr 15, 2015

Thanks @cangermueller and @amueller !

@lesteve
Copy link
Member

lesteve commented Apr 15, 2015

I think you need to git branch -D 4508 just before the git checkout -b:

Not that this is really important, but git branch -m should have renamed (rather than copied) the 4508 branch so the git checkout -b 4508 should work fine.

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

4 participants