/scikit-learn

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] removed precomputed support in nearestcentroid #8515

Merged
merged 4 commits into from May 19, 2017

Conversation

Reviewers

@jnothman jnothman

@amueller amueller

@MechCoder MechCoder

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@sergulaydore @amueller @jmschrei @jnothman @MechCoder
@sergulaydore
Contributor

sergulaydore commented Mar 4, 2017

Reference Issue #8505

What does this implement/fix? Explain your changes.

It is ambiguous what the self.centrodis_ should be when X is already a distance metrics. So, there is no reason to do test for pre-computed matrix.

Any other comments?

This was added in commit cf811d2 by @mblondel
@sergulaydore
removed test_precomputed
Loading status checks…
71ace3a
@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Mar 4, 2017

Codecov Report

Merging #8515 into master will increase coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8515      +/-   ##
==========================================
+ Coverage   95.48%   95.53%   +0.05%     
==========================================
  Files         342      333       -9     
  Lines       60913    61209     +296     
==========================================
+ Hits        58160    58477     +317     
+ Misses       2753     2732      -21
Impacted Files Coverage Δ
sklearn/neighbors/tests/test_nearest_centroid.py 100% <100%> (ø) ⬆️
sklearn/neighbors/nearest_centroid.py 98.48% <100%> (+0.04%) ⬆️
sklearn/metrics/cluster/tests/test_bicluster.py 93.82% <0%> (-6.18%) ⬇️
sklearn/metrics/cluster/bicluster.py 95.18% <0%> (-4.82%) ⬇️
sklearn/feature_selection/base.py 95.56% <0%> (-1.87%) ⬇️
sklearn/feature_selection/tests/test_base.py 98.93% <0%> (-1.07%) ⬇️
sklearn/utils/multiclass.py 95.83% <0%> (-0.7%) ⬇️
sklearn/datasets/twenty_newsgroups.py 25.31% <0%> (-0.47%) ⬇️
sklearn/datasets/base.py 92.43% <0%> (-0.39%) ⬇️
sklearn/metrics/tests/test_common.py 99.16% <0%> (-0.35%) ⬇️
... and 120 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cdd693b...d64fe3f. Read the comment docs.

codecov bot commented Mar 4, 2017 •
edited
Edited 1 time
  • @codecov codecov bot edited May 19, 2017 (most recent)

Codecov Report

Merging #8515 into master will increase coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8515      +/-   ##
==========================================
+ Coverage   95.48%   95.53%   +0.05%     
==========================================
  Files         342      333       -9     
  Lines       60913    61209     +296     
==========================================
+ Hits        58160    58477     +317     
+ Misses       2753     2732      -21
Impacted Files Coverage Δ
sklearn/neighbors/tests/test_nearest_centroid.py 100% <100%> (ø) ⬆️
sklearn/neighbors/nearest_centroid.py 98.48% <100%> (+0.04%) ⬆️
sklearn/metrics/cluster/tests/test_bicluster.py 93.82% <0%> (-6.18%) ⬇️
sklearn/metrics/cluster/bicluster.py 95.18% <0%> (-4.82%) ⬇️
sklearn/feature_selection/base.py 95.56% <0%> (-1.87%) ⬇️
sklearn/feature_selection/tests/test_base.py 98.93% <0%> (-1.07%) ⬇️
sklearn/utils/multiclass.py 95.83% <0%> (-0.7%) ⬇️
sklearn/datasets/twenty_newsgroups.py 25.31% <0%> (-0.47%) ⬇️
sklearn/datasets/base.py 92.43% <0%> (-0.39%) ⬇️
sklearn/metrics/tests/test_common.py 99.16% <0%> (-0.35%) ⬇️
... and 120 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cdd693b...d64fe3f. Read the comment docs.
@amueller

This comment has been minimized.

Show comment
Hide comment
@amueller

amueller Mar 4, 2017

Member

Hm that is really odd. codecov says the coverage decreased, but shows no changes. Looks like a bug in codecov?
Member

amueller commented Mar 4, 2017

Hm that is really odd. codecov says the coverage decreased, but shows no changes. Looks like a bug in codecov?
@jmschrei

This comment has been minimized.

Show comment
Hide comment
@jmschrei

jmschrei Mar 4, 2017

Member

I'm guessing that a unit test counts as a documented part of the code, and so removing it slightly lowers the covered code.
Member

jmschrei commented Mar 4, 2017

I'm guessing that a unit test counts as a documented part of the code, and so removing it slightly lowers the covered code.
@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Mar 5, 2017

Member

But isn't that test trying to define what X should be in the precomputed case, i.e. not make it ambiguous?
Member

jnothman commented Mar 5, 2017

But isn't that test trying to define what X should be in the precomputed case, i.e. not make it ambiguous?
@amueller

This comment has been minimized.

Show comment
Hide comment
@amueller

amueller Mar 5, 2017

Member

Ah, with our settings, tests are included in the coverage check, and removing a test therefore decreases coverage. Maybe we should change our threshold? -0.01 seems pretty strict.
Member

amueller commented Mar 5, 2017

Ah, with our settings, tests are included in the coverage check, and removing a test therefore decreases coverage. Maybe we should change our threshold? -0.01 seems pretty strict.
@sergulaydore

This comment has been minimized.

Show comment
Hide comment
@sergulaydore

sergulaydore Mar 6, 2017

Contributor

@jnothman X is a distance matrix between samples and centroids but we do not know what centroids are. Then the question is how to predict classes for new data samples. If X is supposed to be a pre-computed centroids, that is a different story. We do not need run fit in that case.
Contributor

sergulaydore commented Mar 6, 2017

@jnothman X is a distance matrix between samples and centroids but we do not know what centroids are. Then the question is how to predict classes for new data samples. If X is supposed to be a pre-computed centroids, that is a different story. We do not need run fit in that case.
@amueller

This comment has been minimized.

Show comment
Hide comment
@amueller

amueller Mar 7, 2017

Member

So I would actually raise an error if someone passes "precomputed". I have no idea what it's supposed to do, and the documentation doesn't clarify.
Member

amueller commented Mar 7, 2017

So I would actually raise an error if someone passes "precomputed". I have no idea what it's supposed to do, and the documentation doesn't clarify.
@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Mar 7, 2017

Member
Member

jnothman commented Mar 7, 2017

@sergulaydore

This comment has been minimized.

Show comment
Hide comment
@sergulaydore

sergulaydore Mar 8, 2017

Contributor

OK, now it raises an error if "precomputed" is used as a metric.
Contributor

sergulaydore commented Mar 8, 2017

OK, now it raises an error if "precomputed" is used as a metric.
@jnothman

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Mar 8, 2017

Member
Member

jnothman commented Mar 8, 2017

@jnothman jnothman changed the title from removed test_precomputed to [MRG+1] removed test_precomputed Mar 8, 2017

@jnothman jnothman changed the title from [MRG+1] removed test_precomputed to [MRG+1] removed precomputed support in nearestcentroid Mar 22, 2017

Show outdated Hide outdated sklearn/neighbors/nearest_centroid.py
@@ -133,6 +133,8 @@ def fit(self, X, y):
self.centroids_[cur_class] = np.median(X[center_mask], axis=0)
else:
self.centroids_[cur_class] = csc_median_axis_0(X[center_mask])
elif self.metric == 'precomputed':

This comment has been minimized.

Show comment
Hide comment
@MechCoder

MechCoder Mar 27, 2017

Member

I think we should raise this error earlier?
Copy URL
@MechCoder

MechCoder Mar 27, 2017

Member

I think we should raise this error earlier?

This comment has been minimized.

Show comment
Hide comment
@jnothman

jnothman Mar 27, 2017

Member

I guess so. And perhaps the error message should be more informative and explicitly say precomputed not supported.
Copy URL
@jnothman

jnothman Mar 27, 2017

Member

I guess so. And perhaps the error message should be more informative and explicitly say precomputed not supported.

This comment has been minimized.

Show comment
Hide comment
@sergulaydore

sergulaydore May 17, 2017

Contributor

@MechCoder By earlier, which line do you mean?
Copy URL
@sergulaydore

sergulaydore May 17, 2017

Contributor

@MechCoder By earlier, which line do you mean?

This comment has been minimized.

Show comment
Hide comment
@amueller

amueller May 17, 2017

Member

@sergulaydore do you want to fix this?
Copy URL
@amueller

amueller May 17, 2017

Member

@sergulaydore do you want to fix this?

This comment has been minimized.

Show comment
Hide comment
@sergulaydore

sergulaydore May 17, 2017

Contributor

@amueller Yes but would be great if somebody tells me where to raise the error.
Copy URL
@sergulaydore

sergulaydore May 17, 2017

Contributor

@amueller Yes but would be great if somebody tells me where to raise the error.

This comment has been minimized.

Show comment
Hide comment
@MechCoder

MechCoder May 18, 2017

Member

You can raise it as early as here (https://github.com/sergulaydore/scikit-learn/blob/38ad1d24690fea9f8d1867c2f8b9587c0204268d/sklearn/neighbors/nearest_centroid.py#L98), with an explicit error message saying metric="precomputed" not supported.
Copy URL
@MechCoder

MechCoder May 18, 2017

Member

You can raise it as early as here (https://github.com/sergulaydore/scikit-learn/blob/38ad1d24690fea9f8d1867c2f8b9587c0204268d/sklearn/neighbors/nearest_centroid.py#L98), with an explicit error message saying metric="precomputed" not supported.
@MechCoder

This comment has been minimized.

Show comment
Hide comment
@MechCoder

MechCoder Mar 27, 2017

Member

+1 otherwise, also this might deserve a whatsnew entry.
Member

MechCoder commented Mar 27, 2017

+1 otherwise, also this might deserve a whatsnew entry.
@sergulaydore
raise error earlier
Loading status checks…
d64fe3f

@MechCoder MechCoder merged commit 4397e7e into scikit-learn:master May 19, 2017
5 checks passed

5 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 95.48%)
Details
codecov/project 95.53% (+0.05%) compared to cdd693b
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@MechCoder

This comment has been minimized.

Show comment
Hide comment
@MechCoder

MechCoder May 19, 2017

Member

Thanks!
Member

MechCoder commented May 19, 2017

Thanks!
@sergulaydore

This comment has been minimized.

Show comment
Hide comment
@sergulaydore

sergulaydore May 19, 2017

Contributor
Contributor

sergulaydore commented May 19, 2017

@MechCoder

This comment has been minimized.

Show comment
Hide comment
@MechCoder

MechCoder May 19, 2017

Member
Member

MechCoder commented May 19, 2017

Sundrique added a commit to Sundrique/scikit-learn that referenced this pull request Jun 14, 2017

@sergulaydore @Sundrique
[MRG+1] removed precomputed support in nearestcentroid (#8515) 
* removed test_precomputed

* Revert "removed test_precomputed"

This reverts commit 71ace3a.

* raise an error if metric is precomputed

* raise error earlier
72ea431

dmohns added a commit to dmohns/scikit-learn that referenced this pull request Aug 7, 2017

@sergulaydore @dmohns
[MRG+1] removed precomputed support in nearestcentroid (#8515) 
* removed test_precomputed

* Revert "removed test_precomputed"

This reverts commit 71ace3a.

* raise an error if metric is precomputed

* raise error earlier
0a61d01

dmohns added a commit to dmohns/scikit-learn that referenced this pull request Aug 7, 2017

@sergulaydore @dmohns
[MRG+1] removed precomputed support in nearestcentroid (#8515) 
* removed test_precomputed

* Revert "removed test_precomputed"

This reverts commit 71ace3a.

* raise an error if metric is precomputed

* raise error earlier
bc4c2c3

NelleV added a commit to NelleV/scikit-learn that referenced this pull request Aug 11, 2017

@sergulaydore @NelleV
[MRG+1] removed precomputed support in nearestcentroid (#8515) 
* removed test_precomputed

* Revert "removed test_precomputed"

This reverts commit 71ace3a.

* raise an error if metric is precomputed

* raise error earlier
680e7b6

paulha added a commit to paulha/scikit-learn that referenced this pull request Aug 19, 2017

@sergulaydore @paulha
[MRG+1] removed precomputed support in nearestcentroid (#8515) 
* removed test_precomputed

* Revert "removed test_precomputed"

This reverts commit 71ace3a.

* raise an error if metric is precomputed

* raise error earlier
a6d4a7c

AishwaryaRK added a commit to AishwaryaRK/scikit-learn that referenced this pull request Aug 29, 2017

@sergulaydore @AishwaryaRK
[MRG+1] removed precomputed support in nearestcentroid (#8515) 
* removed test_precomputed

* Revert "removed test_precomputed"

This reverts commit 71ace3a.

* raise an error if metric is precomputed

* raise error earlier
95911ef

maskani-moh added a commit to maskani-moh/scikit-learn that referenced this pull request Nov 15, 2017

@sergulaydore @maskani-moh
[MRG+1] removed precomputed support in nearestcentroid (#8515) 
* removed test_precomputed

* Revert "removed test_precomputed"

This reverts commit 71ace3a.

* raise an error if metric is precomputed

* raise error earlier
2627f7d

jwjohnson314 pushed a commit to jwjohnson314/scikit-learn that referenced this pull request Dec 18, 2017

@sergulaydore Jeremiah Johnson
[MRG+1] removed precomputed support in nearestcentroid (#8515) 
* removed test_precomputed

* Revert "removed test_precomputed"

This reverts commit 71ace3a.

* raise an error if metric is precomputed

* raise error earlier
472b498
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment