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.

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

[Refactor] Classification 1/n #1054

Merged
merged 76 commits into from
Jul 13, 2022
Merged

[Refactor] Classification 1/n #1054

merged 76 commits into from
Jul 13, 2022

Conversation

SkafteNicki
Copy link
Member

@SkafteNicki SkafteNicki commented May 30, 2022 •
edited
Loading

What does this PR do?

Start of the classification refactor #1001 . Initial plan is getting the ConfusionMatrix and StatScores metric done as many other metrics can be derived from them. cc: @Borda, @justusschock

Before submitting

  • Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure to update the docs?
  • Did you write any new necessary tests?

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃

@SkafteNicki SkafteNicki added the refactoring refactoring and code health label May 30, 2022
@SkafteNicki SkafteNicki added this to the v0.10 milestone May 30, 2022
@justusschock
Copy link
Member

@SkafteNicki

1.) IMO we don't need to deprecate them, I thought we were going to have separate metrics per class and still the "old style" classes doing input validation?

2.) I think that's fine. As long as we still have the update and compute interface for the user, more granularity doesn't hurt. Reusing is always good :)

3.) I think that's fine as well as long as it defaults to True and explicitly has to be disabled.

@mergify mergify bot added the ready label Jul 12, 2022
@Borda Borda requested a review from a team July 12, 2022 16:56
@Borda
Copy link
Member

Borda commented Jul 12, 2022

@SkafteNicki seems lat three test are failing, then we shall be fine :)

FAILED unittests/image/test_lpips.py::TestLPIPS::test_lpips_half_gpu[vgg] - T...
FAILED unittests/image/test_lpips.py::TestLPIPS::test_lpips_half_gpu[alex] - ...
FAILED unittests/image/test_lpips.py::TestLPIPS::test_lpips_half_gpu[squeeze]

@mergify mergify bot added ready and removed ready labels Jul 12, 2022
@justusschock
Copy link
Member

Nice, should we merge it to the dev branch for the overall refactor then?

@SkafteNicki SkafteNicki merged commit e69e074 into devel/classification Jul 13, 2022
@SkafteNicki SkafteNicki deleted the refactor/classification_1 branch July 13, 2022 09:45
This was referenced Jul 13, 2022
Merged
Merged
Merged
This was referenced Jul 22, 2022
Merged
Merged
This was referenced Aug 1, 2022
Merged
Merged
@SkafteNicki SkafteNicki mentioned this pull request Aug 20, 2022
Merged
4 tasks
Borda added a commit that referenced this pull request Aug 26, 2022
@SkafteNicki @pre-commit-ci
@stancld @justusschock @Borda
[Refactor] Classification 1/n (#1054)
f06e41e 
* base structure

* bincount

* binary

* files

* stat score

* multiclass + multilabel confmat

* update

* stat_score

* change bincount

* move back

* del tests

* rest of structure

* confmat working

* working binary stat scores

* full testing

* update

* update

* add missing tests

* update

* multilabel stat scores

* disable old testing

* more testing

* flaky tests

* changelog

* refactor

* fixes

* typing

* update

* fix tests

* Apply suggestions from code review

* Apply suggestions from code review

* Apply suggestions from code review

* Update src/torchmetrics/functional/classification/confusion_matrix.py

* missing literal

* add docstring to functional confusion matrix

* add docstring to modular confusion matrix

* add docstring to functional stat scores

* add docstring to modular stat scores

* make private

* docs

* fix mypy and doctests

* fix docs formatting

* literal backwards

* custom movedim

* debug

* debug

* fix tests

* fix tests

* fix tests

* fix tests

* add some testing

* fix tests

* fix docstring

* fix tests

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Daniel Stancl <46073029+stancld@users.noreply.github.com>
Co-authored-by: Justus Schock <12886177+justusschock@users.noreply.github.com>
Co-authored-by: Jirka <jirka.borovec@seznam.cz>
@SkafteNicki SkafteNicki mentioned this pull request Aug 28, 2022
Merged
4 tasks
Borda added a commit that referenced this pull request Sep 12, 2022
@SkafteNicki @pre-commit-ci
@stancld @justusschock @Borda
[Refactor] Classification 1/n (#1054)
9369673 
* base structure

* bincount

* binary

* files

* stat score

* multiclass + multilabel confmat

* update

* stat_score

* change bincount

* move back

* del tests

* rest of structure

* confmat working

* working binary stat scores

* full testing

* update

* update

* add missing tests

* update

* multilabel stat scores

* disable old testing

* more testing

* flaky tests

* changelog

* refactor

* fixes

* typing

* update

* fix tests

* Apply suggestions from code review

* Apply suggestions from code review

* Apply suggestions from code review

* Update src/torchmetrics/functional/classification/confusion_matrix.py

* missing literal

* add docstring to functional confusion matrix

* add docstring to modular confusion matrix

* add docstring to functional stat scores

* add docstring to modular stat scores

* make private

* docs

* fix mypy and doctests

* fix docs formatting

* literal backwards

* custom movedim

* debug

* debug

* fix tests

* fix tests

* fix tests

* fix tests

* add some testing

* fix tests

* fix docstring

* fix tests

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Daniel Stancl <46073029+stancld@users.noreply.github.com>
Co-authored-by: Justus Schock <12886177+justusschock@users.noreply.github.com>
Co-authored-by: Jirka <jirka.borovec@seznam.cz>
Borda added a commit that referenced this pull request Sep 12, 2022
@SkafteNicki @pre-commit-ci
@stancld @justusschock @Borda
[Refactor] Classification 1/n (#1054)
e357655 
* base structure

* bincount

* binary

* files

* stat score

* multiclass + multilabel confmat

* update

* stat_score

* change bincount

* move back

* del tests

* rest of structure

* confmat working

* working binary stat scores

* full testing

* update

* update

* add missing tests

* update

* multilabel stat scores

* disable old testing

* more testing

* flaky tests

* changelog

* refactor

* fixes

* typing

* update

* fix tests

* Apply suggestions from code review

* Apply suggestions from code review

* Apply suggestions from code review

* Update src/torchmetrics/functional/classification/confusion_matrix.py

* missing literal

* add docstring to functional confusion matrix

* add docstring to modular confusion matrix

* add docstring to functional stat scores

* add docstring to modular stat scores

* make private

* docs

* fix mypy and doctests

* fix docs formatting

* literal backwards

* custom movedim

* debug

* debug

* fix tests

* fix tests

* fix tests

* fix tests

* add some testing

* fix tests

* fix docstring

* fix tests

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Daniel Stancl <46073029+stancld@users.noreply.github.com>
Co-authored-by: Justus Schock <12886177+justusschock@users.noreply.github.com>
Co-authored-by: Jirka <jirka.borovec@seznam.cz>
Borda added a commit that referenced this pull request Sep 13, 2022
@SkafteNicki @pre-commit-ci
@stancld @justusschock @Borda
[Refactor] Classification 1/n (#1054)
2ac0fcc 
* base structure

* bincount

* binary

* files

* stat score

* multiclass + multilabel confmat

* update

* stat_score

* change bincount

* move back

* del tests

* rest of structure

* confmat working

* working binary stat scores

* full testing

* update

* update

* add missing tests

* update

* multilabel stat scores

* disable old testing

* more testing

* flaky tests

* changelog

* refactor

* fixes

* typing

* update

* fix tests

* Apply suggestions from code review

* Apply suggestions from code review

* Apply suggestions from code review

* Update src/torchmetrics/functional/classification/confusion_matrix.py

* missing literal

* add docstring to functional confusion matrix

* add docstring to modular confusion matrix

* add docstring to functional stat scores

* add docstring to modular stat scores

* make private

* docs

* fix mypy and doctests

* fix docs formatting

* literal backwards

* custom movedim

* debug

* debug

* fix tests

* fix tests

* fix tests

* fix tests

* add some testing

* fix tests

* fix docstring

* fix tests

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Daniel Stancl <46073029+stancld@users.noreply.github.com>
Co-authored-by: Justus Schock <12886177+justusschock@users.noreply.github.com>
Co-authored-by: Jirka <jirka.borovec@seznam.cz>
Borda added a commit that referenced this pull request Sep 13, 2022
@SkafteNicki @pre-commit-ci
@stancld @justusschock @Borda
[Refactor] Classification 1/n (#1054)
b7120e3 
* base structure

* bincount

* binary

* files

* stat score

* multiclass + multilabel confmat

* update

* stat_score

* change bincount

* move back

* del tests

* rest of structure

* confmat working

* working binary stat scores

* full testing

* update

* update

* add missing tests

* update

* multilabel stat scores

* disable old testing

* more testing

* flaky tests

* changelog

* refactor

* fixes

* typing

* update

* fix tests

* Apply suggestions from code review

* Apply suggestions from code review

* Apply suggestions from code review

* Update src/torchmetrics/functional/classification/confusion_matrix.py

* missing literal

* add docstring to functional confusion matrix

* add docstring to modular confusion matrix

* add docstring to functional stat scores

* add docstring to modular stat scores

* make private

* docs

* fix mypy and doctests

* fix docs formatting

* literal backwards

* custom movedim

* debug

* debug

* fix tests

* fix tests

* fix tests

* fix tests

* add some testing

* fix tests

* fix docstring

* fix tests

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Daniel Stancl <46073029+stancld@users.noreply.github.com>
Co-authored-by: Justus Schock <12886177+justusschock@users.noreply.github.com>
Co-authored-by: Jirka <jirka.borovec@seznam.cz>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justusschock justusschock justusschock approved these changes

@awaelchli awaelchli awaelchli approved these changes

@tchaton tchaton Awaiting requested review from tchaton

@ethanwharris ethanwharris Awaiting requested review from ethanwharris

@SeanNaren SeanNaren Awaiting requested review from SeanNaren

@Borda Borda Awaiting requested review from Borda

@williamFalcon williamFalcon Awaiting requested review from williamFalcon

@edenlightning edenlightning Awaiting requested review from edenlightning

@stancld stancld Awaiting requested review from stancld

Assignees

@SkafteNicki SkafteNicki

Labels
ready refactoring refactoring and code health
Projects
None yet
Milestone
v0.10
Development

Successfully merging this pull request may close these issues.
5 participants
@SkafteNicki @justusschock @Borda @awaelchli @stancld