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

Adding quality metrics and opening discussion #3912

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

alexdesiqueira
Copy link
Member

@alexdesiqueira alexdesiqueira commented May 16, 2019

Description

This brings us a basic implementation for several metrics: precision, recall, specificity, accuracy, and the coefficients of Matthews, Dice (F1 score), and informedness. Open to discussion (see first comment).

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.
  • Consider backporting the PR with @meeseeksdev backport to v0.14.x

@pep8speaks
Copy link

pep8speaks commented May 16, 2019

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

Line 180:80: E501 line too long (111 > 79 characters)
Line 216:80: E501 line too long (81 > 79 characters)
Line 270:13: E226 missing whitespace around arithmetic operator
Line 270:25: E226 missing whitespace around arithmetic operator
Line 297:80: E501 line too long (89 > 79 characters)
Line 299:80: E501 line too long (80 > 79 characters)
Line 340:80: E501 line too long (81 > 79 characters)
Line 353:55: W504 line break after binary operator
Line 381:80: E501 line too long (80 > 79 characters)
Line 419:80: E501 line too long (80 > 79 characters)
Line 457:80: E501 line too long (80 > 79 characters)

Comment last updated at 2020-02-28 00:08:14 UTC

@alexdesiqueira
Copy link
Member Author

Hi @scikit-image/core,
I implemented these functions a while ago, and since we don't have them, I'd like to discuss their addition. For that, some questions:

  1. Do we need these metrics? I think scikit-learn has some of them. Please feel free to close the PR if they are not worth the effort.
    Is the PR still alive? OK, the other issues:
  2. They are scattered around one file. I think they maybe would be better implemented as one class instead, that could also return a confusion matrix. Thoughts?
  3. This implementation only works on binary images right now. I am not working on cases with labels, or fancier images; that would need more effort. sklearn has their confusion matrix as something like that, however (it works on several classes). Maybe it could be a start.
    After discussing this I'd work on documentation and all.
    Thank you for your time.

@alexdesiqueira
Copy link
Member Author

  1. This implementation only works on binary images right now. I am not working on cases with labels, or fancier images; that would need more effort. sklearn has their confusion matrix as something like that, however (it works on several classes). Maybe it could be a start.

On the other hand, some of these measures only work on binary images... silly me 😌

@stefanv
Copy link
Member

stefanv commented May 16, 2019

There seems to be a few segmentation/clustering comparison algorithms out there, including Normalized Probabilistic Rand: https://ieeexplore.ieee.org/abstract/document/1565332. I guess there are others too; we should try to find a review paper.

@alexdesiqueira
Copy link
Member Author

@stefanv I have one or two reviews I used during previous research; I'll paste their names and possible links soon.

@stefanv
Copy link
Member

stefanv commented May 21, 2019 via email

@alexdesiqueira
Copy link
Member Author

I missed it 🤦‍♂️ I saw only confusion_matrix before. I believe they all are usable for our images, and I think they're prepared to deal with more than two classes.

@stefanv
Copy link
Member

stefanv commented May 22, 2019 via email

@alexdesiqueira
Copy link
Member Author

Closed due to overlapping with scikit-learn functions. Please reopen if necessary.

@jni
Copy link
Member

jni commented May 28, 2019

I think I'd like this reopened. I'd also appreciate reviews on #3354!

@alexdesiqueira
Copy link
Member Author

@jni any opinions on this one?

@rbavery rbavery mentioned this pull request Jul 14, 2019
10 tasks
@alexdesiqueira
Copy link
Member Author

Reopening this; @jni, would you like to discuss that?

Base automatically changed from master to main February 18, 2021 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants