Add averaging option to AMI and NMI #11124

Merged
merged 15 commits into from Jul 17, 2018

Conversation

Projects
6 participants
Contributor

aryamccarthy commented May 23, 2018 • edited

 Reference Issues/PRs See #10308; this is a first step toward eventually deprecating one behavior and making their behavior consistent. What does this implement/fix? Background: The measures AMI, NMI, and V-measure are intimately related. Each is a normalized version of mutual information, and AMI incorporates adjustment for chance. AMI, NMI, and V-Measure use different strategies for normalizing: the arithmetic mean, geometric mean, and max (i.e. infinity-norm) of the two clusterings' entropies. (V-measure is NMI with arithmetic mean.) This makes the measures difficult to directly compare. Added switch for NMI and AMI to allow choice of normalization Long-term plan: unify behavior. Warning about future deprecation.
 Add averaging option to AMI and NMI 
Leave current behavior unchanged
 80844ae 

amueller reviewed May 24, 2018

Needs tests, otherwise great!

sklearn/metrics/cluster/supervised.py Outdated
sklearn/metrics/cluster/supervised.py Outdated
Member

amueller commented May 24, 2018

 test failures are flake8. you should run flake8 in your editor.
Member

amueller commented May 24, 2018

 oh this is related I just saw #8645

aryamccarthy added some commits May 24, 2018

 Flake8 fixes 
 4794486 
 Incorporate tests of means for AMI and NMI 
 6279c25 

amueller reviewed May 24, 2018

sklearn/metrics/cluster/supervised.py Outdated

amueller reviewed May 24, 2018

sklearn/metrics/cluster/supervised.py Outdated

aryamccarthy added some commits May 24, 2018

 Add note about average_method in NMI 
 ed500d6 
 Update docs from AMI, NMI changes (#1) 
* Correct the NMI and AMI descriptions in docs

* Update docstrings due to averaging changes

- V-measure
- Homogeneity
- Completeness
- NMI
- AMI
 5ed8527 
Contributor

aryamccarthy commented May 25, 2018

 Looks ready to squash and merge—the fix is implemented and the tests passed. @amueller ?

jnothman reviewed May 26, 2018

Not so fast ;)

 @@ -1185,7 +1179,7 @@ following equation, from Vinh, Epps, and Bailey, (2009). In this equation, Using the expected value, the adjusted mutual information can then be calculated using a similar form to that of the adjusted Rand index: .. math:: \text{AMI} = \frac{\text{MI} - E[\text{MI}]}{\max(H(U), H(V)) - E[\text{MI}]} .. math:: \text{AMI} = \frac{\text{MI} - E[\text{MI}]}{\text{mean}(H(U), H(V)) - E[\text{MI}]}

jnothman May 26, 2018

Member

The fact that mean is configurable and varies in the literature should be discussed here, perhaps with some notes on when one is more appropriate than another

sklearn/metrics/cluster/tests/test_supervised.py Outdated
Member

jnothman commented May 26, 2018

 Please add an entry to the change log at doc/whats_new/v0.20.rst. Like the other entries there, please reference this pull request with :issue: and credit yourself (and other contributors if applicable) with :user:

aryamccarthy added some commits May 27, 2018

 Update documentation and remove nose tests (#2) 
* Update v0.20.rst

* Update test_supervised.py

* Update clustering.rst
 df60d46 
 Fix multiple spaces after operator 
 b449cb9 
Contributor

aryamccarthy commented May 27, 2018

 Mission accomplished :)

jnothman reviewed May 27, 2018

I think this is a bit confusing. The normalising constant is always the max of some elementwise mean of U and V.

sqrt and sum don't make sense as names of means: call them geometric and arithmetic

Contributor

aryamccarthy commented May 27, 2018

 Right—I wanted to stick to the notation started by Vinh et al and used elsewhere, but the names are awful. I can switch them. … -am On May 27, 2018, 6:46 PM -0400, Joel Nothman ***@***.***>, wrote: @jnothman commented on this pull request. I think this is a bit confusing. The normalising constant is always the max of some elementwise mean of U and V. sqrt and sum don't make sense as names of means: call them geometric and arithmetic — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.
Member

jnothman commented May 28, 2018

 You can provide a mapping from reasonable names to Vinh in the docs. Thanks
 Rename all arguments 
 1b36da5 
Contributor

aryamccarthy commented Jun 5, 2018

 Checking again—is this ready to pull?

jnothman reviewed Jun 6, 2018

Not yet looked at tests

doc/whats_new/v0.20.rst Outdated
sklearn/metrics/cluster/supervised.py Outdated
sklearn/metrics/cluster/supervised.py Outdated
doc/modules/clustering.rst Outdated
sklearn/metrics/cluster/supervised.py Outdated
sklearn/metrics/cluster/supervised.py Outdated

aryamccarthy added some commits Jun 6, 2018

 No more arbitrary values! 
 3d8bf2c 
 Improve handling of floating-point imprecision 
 2854014 

jnothman reviewed Jun 6, 2018

sklearn/metrics/cluster/supervised.py Outdated
sklearn/metrics/cluster/supervised.py Outdated
 Clearly state when the change occurs 
 059bae6 
Contributor

aryamccarthy commented Jun 9, 2018

 @jnothman or @amueller, is this ready?

jnothman approved these changes Jun 9, 2018

Yes, I think this looks good now.

Contributor

aryamccarthy commented Jun 9, 2018

 Ah, then pull away! ;)
Member

jnothman commented Jun 9, 2018

 We require two approvals before merge (sorry!) Hopefully @amueller can give this another glance.
Contributor

Member

jnothman commented Jul 12, 2018

 This might get some attention with the sprints next week, but probably not from Hanmin.
Member

qinhanmin2014 commented Jul 12, 2018

 Apologies for the delay and thanks @aryamccarthy for your great work. I'll mark this as 0.20 to help you attract reviewers. For me, I can only promise to give a review after the release.

amueller approved these changes Jul 12, 2018

There's probably lots of deprecation warnings in the tests now. Can you please either catch them or explicitly pass the new parameter? (do we have a standard procedure for this btw? Is it documented?)

doc/modules/clustering.rst Outdated
doc/modules/clustering.rst Outdated
doc/modules/clustering.rst Outdated
doc/modules/clustering.rst Outdated
doc/whats_new/v0.20.rst Outdated
doc/whats_new/v0.20.rst Outdated
 normalized_mutual_info_score, adjusted_mutual_info_score, ] means = {"min", "geometric", "arithmetic", "max"}

amueller Jul 12, 2018

Member

feel kinda weird about calling these means.

aryamccarthy Jul 12, 2018 • edited

Contributor

I can switch to generalized_means if you prefer, but it has to be clear that this is a specific class of aggregations. Product, for instance, wouldn't work. Let me know and I'll ship all changes in one PR update.

 Update AMI/NMI docs 
 e8b9579 
Member

amueller commented Jul 15, 2018

 did you check that you're catching all deprecation warnings?
Contributor

aryamccarthy commented Jul 15, 2018

 With this? with warnings.catch_warnings(): warnings.filterwarnings("ignore",category=PendingDeprecationWarning) Also someone pushed in a way that introduces merge conflicts in the whats-new file, so tests aren't passing.
Member

amueller commented Jul 15, 2018

 can you merge master to fix the conflict? And I think we're using  with ignore_warnings(category=DeprecationWarning):  right now.

amueller moved this from PRs tagged to Blockers in scikit-learn 0.20Jul 16, 2018

 Merge branch 'master' into pr/11124 
 afe9776 
Member

amueller commented Jul 16, 2018

 fixed the conflict
Contributor

aryamccarthy commented Jul 16, 2018

 For my future knowledge, what's the command to do that?
Member

amueller commented Jul 16, 2018

 I merged master into it and fixed the merge conflict and pushed into your branch. So assuming you're on your branch and have the main repo as upstream remote git pull upstream master # fix merge conflict, commit git push origin master  fyi you shouldn't send PRs from your master branch, you should ideally create a feature branch.
 Update v0.20.rst 
 c65d2b3 
Member

amueller commented Jul 16, 2018

 can you please also add a test that there are deprecation warnings? Also see the updated docs at http://scikit-learn.org/dev/developers/contributing.html#change-the-default-value-of-a-parameter
Contributor

aryamccarthy commented Jul 16, 2018

 I've written (but not committed) that. My concern is catching all of the FutureWarnings. I'd have to infect the entire test_supervised.py file with with ignore_warnings(category=FutureWarning):.
 Catch FutureWarnings in AMI and NMI 
 a5b3c0f 
Contributor

massich commented Jul 17, 2018

 LGTM +1 to merge
Member

GaelVaroquaux commented Jul 17, 2018

 LGTM. Merging.

GaelVaroquaux merged commit 52b6a66 into scikit-learn:master Jul 17, 2018 7 checks passed

7 checks passed

ci/circleci: deploy Your tests passed on CircleCI!
Details
ci/circleci: python2 Your tests passed on CircleCI!
Details
ci/circleci: python3 Your tests passed on CircleCI!
Details
codecov/patch 98.48% of diff hit (target 95.36%)
Details
codecov/project 95.37% (+<.01%) compared to 726fa36
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Member

jnothman commented Jul 18, 2018

 Thanks Arya!​

Closed

Closed

Closed