8000 Fix variation_of_information by rfezzani · Pull Request #4875 · scikit-image/scikit-image · GitHub
[go: up one dir, main page]
Skip to content

Fix variation_of_information #4875

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 8 commits into from
Aug 4, 2020
Merged

Fix variation_of_information #4875

merged 8 commits into from
Aug 4, 2020

Conversation

rfezzani
Copy link
Member

Description

Fixes #4856

The original bug is in the contingency table normalization.

Checklist

For reviewers

  • Check that the PR title is short, concise, and will make sense 1 year
    later.
  • Check that new functions are imported in corresponding __init__.py.
  • Check that new features, API changes, and deprecations are mentioned in
    doc/release/release_dev.rst.

@rfezzani rfezzani added the 🩹 type: Bug fix Fixes unexpected or incorrect behavior label Jul 31, 2020
@pep8speaks
Copy link
pep8speaks commented Jul 31, 2020 •
edited
Loading

Hello @rfezzani! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-07-31 10:17:47 UTC

jni
jni approved these changes Jul 31, 2020
View reviewed changes
Copy link
Member
@jni jni left a comment

Choose a reason for hiding this comment

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

Very clever! I really like your reimplementation, though it took me a bit longer to find the actually relevant change. =P

I don't really like that you changed ignore_labels to None, but I won't hold up the PR over it. =P

@jni
Copy link
Member
jni commented Jul 31, 2020

Not sure why pooch is failing in some builds with:

2020-07-31T10:27:45.5728429Z E ValueError: SHA256 hash of downloaded file (cells.tif) does not match the known hash: expected 2120cfe08e0396324793a10a905c9bbcb64b117215eb63b2c24b643e1600c8c9 but got ce67cfa98abd07c0ed4cedeac795ab118a65aa166d5ddbeb6fad756793c3bb4a. Deleted download for safety. The downloaded file may have been corrupted or the known hash may be outdated.

I guess we can ignore as not relevant to this PR!

@rfezzani
Copy link
Member Author
rfezzani commented Jul 31, 2020 •
edited
Loading

Thank you @jni, sorry for changing the value of ignore_labels from () to None, but it really jumped to my eyes and I couldn't resist ^^

emmanuelle
emmanuelle reviewed Aug 4, 2020
View reviewed changes
skimage/metrics/_contingency_table.py
data[ignored] = 0
if ignore_labels is None:
ignore_labels = []
im_test_r = im_test.reshape(-1)
Copy link
Member

Choose a reason for hiding this comment

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

just curious to know why you changed im.ravel() to im.reshape(-1)?

(it's just for my own curiosity, no problem with the proposal)

Copy link
Member

Choose a reason for hiding this comment

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

@emmanuelle The answer is in the .ravel docstring: .reshape is less likely to result in a copy. Which surprised me but after several reminders @rfezzani has finally hammered it into my brain.

Copy link
Member

Choose a reason for hiding this comment

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

now that you say it I'm almost sure I already asked @rfezzani this same question. Thanks for answering, we're doing group learning here :-).

@emmanuelle
Copy link
Member

Thanks @rfezzani, merging.

@emmanuelle emmanuelle merged commit 1cc2776 into scikit-image:master Aug 4, 2020
@sciunto sciunto mentioned this pull request Aug 10, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emmanuelle emmanuelle emmanuelle left review comments

@jni jni jni approved these changes

Assignees
No one assigned
Labels
🩹 type: Bug fix Fixes unexpected or incorrect behavior
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Problem in variation of infromation, should we ignore 0 in pred?
4 participants
@rfezzani @pep8speaks @jni @emmanuelle
0