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

Adjusted Mutual Information #402

Merged
merged 38 commits into from Nov 10, 2011
Merged

Conversation

robertlayton
Copy link
Member

Mutual Information, adjusted for chance. See [1] for details (specifically, look at the references for detail)

I have tested this against the Matlab code, and it works! Took me a while, as I had log for entropy and log2 for Expected Mutual Information (should be the other way around). I think that the _expected_mutual_information can be optimised, but I went with "get it right" first.

[1] http://en.wikipedia.org/wiki/Adjusted_Mutual_Information

@ogrisel
Copy link
Member

ogrisel commented Oct 19, 2011

Thanks for this contrib!
Can you please merge the current master to your branch to make it easier for reviewers to test your code?



def expected_mutual_information(contingency, n_samples):
""" Calculate the expected mutual information for two labellings. """
Copy link
Member

Choose a reason for hiding this comment

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

Typo (the U.S. spelling with a single "l" is more common) and PEP257 style:

"""Calculate the expected mutual information for two labelings."""

@ogrisel
Copy link
Member

ogrisel commented Oct 19, 2011

I have added new items to the TODO list.

Conflicts:
	sklearn/metrics/cluster/__init__.py
- See Also sections updated
- mutual_information -> mutual_information_score (and updating subsequent imports)
…sted_for_chance_measures.py example

Commiting to get help on error
@robertlayton
Copy link
Member Author

There is an overflow problem, which happens when running examples/cluster/plot_adjusted_for_chance_measures.py. I'm not quite sure how to fix this. Any suggestions? My thinking is to rearrange the third term of the equation, or to skip when a[i]== 0 etc.
edit: I understand why the matlab code is how it is - reducing factorials etc. I just worked out the equations to reduce the number of factorials needed and I'll do that for now.

edit #2: I think I've fixed it. I need to test against the matlab code with larger arrays, but I have to do that on my other computer.

@robertlayton
Copy link
Member Author

I'm not familiar with the style of plots used in the plot_adjusted_for_chance_measures.py example. Can someone show me how to make the colours of the legend match with the actual plots? They are just showing up as a broken line on my machine.

@robertlayton
Copy link
Member Author

Apart from the issue with plot_adjusted_for_chance_measures.py, the rest of this code is ready for a more thorough review.

@ogrisel
Copy link
Member

ogrisel commented Oct 21, 2011

Sorry for the late reply, I will try to have a look at the plot_adjusted_for_chance_measures stuff tomorrow.

@ogrisel
Copy link
Member

ogrisel commented Oct 22, 2011

Here are the pictures I get:

Random vs Random with 100 samples
Random vs Fixed (10 centers) with 1000 samples

The legends are good. However the AMI score should always be zero (as the ARI) which is not the case. This might be caused by the following warnings I get when computing the AMI scores:

Warning: divide by zero encountered in log
Warning: invalid value encountered in multiply

Also it seems that the AMI scores are much slower to compute than ARI and V-Measure. Would be great to try and optimize the runtime once the incorrect value issues are fixed.

@ogrisel
Copy link
Member

ogrisel commented Oct 22, 2011

Also here is the outcome of the doctests:

nosetests -s --with-doctest --doctest-tests --doctest-extension=rst \
    --doctest-fixtures=_fixture doc/ doc/modules/
.Warning: divide by zero encountered in log
Warning: invalid value encountered in multiply
Warning: divide by zero encountered in log
Warning: invalid value encountered in multiply
Warning: divide by zero encountered in log
Warning: invalid value encountered in multiply
Warning: divide by zero encountered in log
Warning: invalid value encountered in multiply
Warning: divide by zero encountered in log
Warning: invalid value encountered in multiply
F..........
======================================================================
FAIL: Doctest: clustering.rst
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/lib/python2.7/doctest.py", line 2166, in runTest
    raise self.failureException(self.format_failure(new.getvalue()))
AssertionError: Failed doctest test for clustering.rst
  File "/home/ogrisel/coding/scikit-learn/doc/modules/clustering.rst", line 0

----------------------------------------------------------------------
File "/home/ogrisel/coding/scikit-learn/doc/modules/clustering.rst", line 491, in clustering.rst
Failed example:
    metrics.ami_score(labels_true, labels_pred)  # doctest: +ELLIPSIS
Expected:
    0.24...
Got:
    0.22504228319830885
----------------------------------------------------------------------
File "/home/ogrisel/coding/scikit-learn/doc/modules/clustering.rst", line 498, in clustering.rst
Failed example:
    metrics.ami_score(labels_true, labels_pred)  # doctest: +ELLIPSIS
Expected:
    0.24...
Got:
    0.22504228319830885
----------------------------------------------------------------------
File "/home/ogrisel/coding/scikit-learn/doc/modules/clustering.rst", line 505, in clustering.rst
Failed example:
    metrics.ami_score(labels_pred, labels_true)  # doctest: +ELLIPSIS
Expected:
    0.24...
Got:
    0.22504228319830885
----------------------------------------------------------------------
File "/home/ogrisel/coding/scikit-learn/doc/modules/clustering.rst", line 518, in clustering.rst
Failed example:
    metrics.ami_score(labels_true, labels_pred)  # doctest: +ELLIPSIS
Expected:
    0.0...
Got:
    -0.10526315789473678

>>  raise self.failureException(self.format_failure(<StringIO.StringIO instance at 0x241bd40>.getvalue()))


----------------------------------------------------------------------
Ran 12 tests in 1.360s

@robertlayton
Copy link
Member Author

Don't check this yet - I didn't commit one lot from home and need to do that first (it updated the examples, which don't work now!)

@robertlayton
Copy link
Member Author

Ready to be checked. I added a note in the docstring of adjusted_mutual_info_score. If you have a better spot, let me know.

@@ -19,7 +19,7 @@ Changelog
- Faster tests by `Fabian Pedregosa`_.

- Silhouette Coefficient cluster analysis evaluation metric added as
``sklearn.metrics.silhouette_score`` by Robert Layton.
``sklearn.metrics.silhouette_score`` by `Robert Layton`.
Copy link
Member

Choose a reason for hiding this comment

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

If you to make your name a link, you need to add a trailing _ to it.

@ogrisel
Copy link
Member

ogrisel commented Nov 10, 2011

Looks good. +1 for merge.

@robertlayton
Copy link
Member Author

Merge when ready.

@GaelVaroquaux
Copy link
Member

Merge when ready.

I am doing changes on this pull request right now :)

robertlayton added a commit that referenced this pull request Nov 10, 2011
Adjusted Mutual Information: Mutual information adjusted for chance
@robertlayton robertlayton merged commit 6263124 into scikit-learn:master Nov 10, 2011
@GaelVaroquaux
Copy link
Member

I made a mistake sending my mail, and it didn't get to the pull request.
So I am resending. In brief, you merged code that is failing a lot of
tests on my box (I tried again after your merge) :(.

--Earlier mail--

I was about to merge, after integrating the changes that I made on my
'ami' branch, but I found that I get quite a few test failures.
@robertlayton, could you please merge my ami branch, and check if you can
reproduce the following failures:

nosetests -s --with-doctest --doctest-tests --doctest-extension=rst \
    --doctest-fixtures=_fixture doc/ doc/modules/
  DeprecationWarning)
.F......F...
======================================================================
FAIL: Doctest: clustering.rst
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/lib/python2.6/doctest.py", line 2163, in runTest
    raise self.failureException(self.format_failure(new.getvalue()))
AssertionError: Failed doctest test for clustering.rst
  File "/home/varoquau/dev/scikit-learn/doc/modules/clustering.rst", line
0

----------------------------------------------------------------------
File "/home/varoquau/dev/scikit-learn/doc/modules/clustering.rst", line
493, in clustering.rst
Failed example:
    metrics.adjusted_mutual_info_score(labels_true, labels_pred)  #
doctest: +ELLIPSIS
Expected:
    0.24...
Got:
    0.22504228319830874
----------------------------------------------------------------------
File "/home/varoquau/dev/scikit-learn/doc/modules/clustering.rst", line
500, in clustering.rst
Failed example:
    metrics.adjusted_mutual_info_score(labels_true, labels_pred)  #
doctest: +ELLIPSIS
Expected:
    0.24...
Got:
    0.22504228319830874
----------------------------------------------------------------------
File "/home/varoquau/dev/scikit-learn/doc/modules/clustering.rst", line
507, in clustering.rst
Failed example:
    metrics.adjusted_mutual_info_score(labels_pred, labels_true)  #
doctest: +ELLIPSIS
Expected:
    0.24...
Got:
    0.22504228319830874
----------------------------------------------------------------------
File "/home/varoquau/dev/scikit-learn/doc/modules/clustering.rst", line
520, in clustering.rst
Failed example:
    metrics.adjusted_mutual_info_score(labels_true, labels_pred)  #
doctest: +ELLIPSIS
Expected:
    0.0...
Got:
Got:
    -0.10526315789473642

>>  raise self.failureException(self.format_failure(>  instance at 0x9c86b8c>.getvalue()))

@robertlayton
Copy link
Member Author

Checking now

(I re-read your comment - I thought you meant "merge when ready, I'm already working off a branch of this"! Sorry about that).

@GaelVaroquaux
Copy link
Member

Checking now

Thanks. I've merged the changes that I had made in my fork of your branch
in master.

@robertlayton
Copy link
Member Author

Issue is confirmed, which is weird - it only happened after I merged the branches :S

I think -- this may be a problem with some of my libraries. I had an issue previously where doctests weren't showing up, despite everything else working. I'll fix that problem independently.

I have the matlab code for AMI, so I'll double check the doctests versus that code.

@GaelVaroquaux
Copy link
Member

Issue is confirmed, which is weird - it only happened after I merged the branches :S

The good news is that you can reproduce it. :$. Maybe you can do a
bisection to find out what commit exactly is to blame.

@robertlayton
Copy link
Member Author

After checking with the matlab code, these values are wrong, but the code is right!

Checking the build now. Do I start a new PR?

@GaelVaroquaux
Copy link
Member

After checking with the matlab code, these values are wrong, but the
code is right!

So, you are saying that the tests are wrong?

Checking the build now. Do I start a new PR?

Don't start a new PR: this is bug fixing, and I don't think that it needs
a new PR. If you want, you can push a fix to your fork, and ask for
informal feedback, but if you have a fix that you a confident with, you
should push it to master. I don't like having failing tests in master: I
am worried about the broken window effect.

@robertlayton
Copy link
Member Author

Yup, tests were wrong. I'll look back over the code to work out why I did that, but its more important just to get the fix in, so that the build is good (as you said).

The fix works (just the values are changed, and I took them straight from matlab). I'll push to master in a second (I'll double check it won't break anything else!).

@GaelVaroquaux
Copy link
Member

Yup, tests were wrong.

Fair enough. Looks like we don't run the tests enough when reviewing pull
requests :)

@GaelVaroquaux
Copy link
Member

Fair enough. Looks like we don't run the tests enough when reviewing pull
requests :)

If it gives you any comfort, I pretty much did the same thing in my pull
request, and forgot to rerun the tests after implementing a change. I was
about to pull to master, and routinely ran the tests... and found the bug
:)

@robertlayton
Copy link
Member Author

That does give me comfort. Thanks for checking though -- broken tests are bad!

@ogrisel It was my understanding that the AMI is bounded between 0 and 1. However one of the examples in doctests gives a negative score -- and it does with the matlab code as well! Any thoughts?

@ogrisel
Copy link
Member

ogrisel commented Nov 10, 2011

Negative score can happen for "random-like" labelings because with are doing a diff with the expected value of random labelings. It is just never occurring in practice if your are evaluating a cluster algorithm that does its job of finding slightly above average clustering.

@robertlayton
Copy link
Member Author

Fair enough. That had me a little worried (but it was what the matlab code had...)

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

5 participants