8000 MRG: Explicitly mark the array returned by the linear svm readonly property coef_ as immutable by ogrisel · Pull Request #541 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

MRG: Explicitly mark the array returned by the linear svm readonly property coef_ as immutable #541

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Jan 7, 2012
Merged

MRG: Explicitly mark the array returned by the linear svm readonly property coef_ as immutable #541

merged 5 commits into from
Jan 7, 2012

Conversation

ogrisel
Copy link
Member
@ogrisel ogrisel commented Jan 7, 2012

This pull-request #539 should be taken into account to understand the context about the debate of readonly properties in the scikit-learn API.

This pull request is also related to issue #470 LogisticRegression and LinearSVC should have read-write coef_ and intercept_ attributes (it does not fix it but makes it a less dangerous behavior).

The following is an example session were the unsuspecting user might overlook a bug:

>>> from sklearn.svm import SVC
>>> from sklearn.datasets import load_iris
>>> iris = load_iris()
>>> clf = SVC(kernel='linear').fit(iris.data, iris.target)
>>> clf.coef_

array([[-1.0443518 ,  0.01195155, -1.91966087, -0.81040704],
       [ 1.5863679 ,  1.66207276, -1.65274791, -1.95241805]])
>>> clf.coef_[0, 3] = 0
>>> clf.coef_

array([[-1.0443518 ,  0.01195155, -1.91966087, -0.81040704],
       [ 1.5863679 ,  1.66207276, -1.65274791, -1.95241805]])

@pprett
Copy link
Member
pprett commented Jan 7, 2012

looks good to me - this will spare us some effort on the mailing list :-) - I'm +1 for merge

@amueller
Copy link
Member
amueller commented Jan 7, 2012

Didn't test but looks good. +1 for merge

GaelVaroquaux
GaelVaroquaux reviewed Jan 7, 2012
View reviewed changes
sklearn/svm/base.py
if len(self.label_) <= 2:
return -ret
ret = -ret
Copy link
Member

Choose a reason for hiding this comment

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

ret *= -1 avoid a copy.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch.

@GaelVaroquaux
Copy link
Member
GaelVaroquaux A210 commented Jan 7, 2012

Appart from the inline comment, I am +1 for a merge

ogrisel added a commit that referenced this pull request Jan 7, 2012
@ogrisel
Merge pull request #541 from ogrisel/immutable-readonly-coef
26632b6 
MRG: Explicitly mark the array returned by the linear svm readonly property coef_ as immutable
@ogrisel ogrisel merged commit 26632b6 into scikit-learn:master Jan 7, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ogrisel @pprett @amueller @GaelVaroquaux
0