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

Refactor/classification 10 #1197

Merged
merged 21 commits into from
Sep 12, 2022
Merged

Conversation

SkafteNicki
Copy link
Member

@SkafteNicki SkafteNicki commented Aug 28, 2022

What does this PR do?

Continues work on classification refactor #1001
Prior work:

This PR is introducing the changes for the transition (deprecation) of the old API to the new.

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 🙃

@Borda Borda added the refactoring refactoring and code health label Aug 30, 2022
Copy link
Member

@Borda Borda left a comment

Choose a reason for hiding this comment

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

Shall we also remove all the commented-out code like tests?

@SkafteNicki
Copy link
Member Author

Shall we also remove all the commented-out code like tests?

Yes.
There may be some of the tests that we want to reintroduce but we should still be able to find them in the future.

@Borda
Copy link
Member

Borda commented Sep 5, 2022

@SkafteNicki still draft? 😇

@SkafteNicki SkafteNicki marked this pull request as ready for review September 8, 2022 06:22
@SkafteNicki SkafteNicki added this to the v0.10 milestone Sep 8, 2022
@codecov
Copy link

codecov bot commented Sep 8, 2022

Codecov Report

Merging #1197 (84c4f1d) into devel/classification (6b60bf3) will decrease coverage by 39%.
The diff coverage is 17%.

@@                  Coverage Diff                   @@
##           devel/classification   #1197     +/-   ##
======================================================
- Coverage                    89%     50%    -39%     
======================================================
  Files                       191     191             
  Lines                     10786   11201    +415     
======================================================
- Hits                       9577    5632   -3945     
- Misses                     1209    5569   +4360     

@SkafteNicki
Copy link
Member Author

@Borda ready for review

src/torchmetrics/classification/average_precision.py Outdated Show resolved Hide resolved
src/torchmetrics/classification/auroc.py Outdated Show resolved Hide resolved
src/torchmetrics/classification/binned_precision_recall.py Outdated Show resolved Hide resolved
src/torchmetrics/classification/binned_precision_recall.py Outdated Show resolved Hide resolved
src/torchmetrics/classification/binned_precision_recall.py Outdated Show resolved Hide resolved
src/torchmetrics/classification/calibration_error.py Outdated Show resolved Hide resolved
@Borda
Copy link
Member

Borda commented Sep 9, 2022

Could you pls check the typing?

@mergify mergify bot added the ready label Sep 9, 2022
@SkafteNicki
Copy link
Member Author

@Borda not sure what to do about the typing. Take jaccard_score as an example:

def jaccard_index(
    preds: Tensor,
    target: Tensor,
    num_classes: int,
    average: Optional[str] = "macro",
    ignore_index: Optional[int] = None,
    absent_score: float = 0.0,
    threshold: float = 0.5,
    task: Optional[Literal["binary", "multiclass", "multilabel"]] = None,  # new arg
    num_labels: Optional[int] = None,  # new arg
    validate_args: bool = True,  # new arg
) -> Tensor:
    if task is not None:
        kwargs = dict(ignore_index=ignore_index, validate_args=validate_args)
        if task == "binary":
            return binary_jaccard_index(preds, target, threshold, **kwargs)
        if task == "multiclass":
            return multiclass_jaccard_index(preds, target, num_classes, average, **kwargs)
        if task == "multilabel":
            return multilabel_jaccard_index(preds, target, num_labels, threshold, average, **kwargs)
        raise ValueError(
            f"Expected argument `task` to either be `'binary'`, `'multiclass'` or `'multilabel'` but got {task}"
        )
    else:
        rank_zero_warn(
            ...
            DeprecationWarning,
        )
    confmat = _confusion_matrix_update(preds, target, num_classes, threshold)
    return _jaccard_from_confmat(confmat, num_classes, average, ignore_index, absent_score)

The new argument num_labels is an optional argument because it is not necessary for the old implementation or in the case of task='binary' and task='multiclass', but it is an required parameter if user set task='multilabel'. Implementation wise should it be pretty safe to call it like this because all arguments are validated inside the sub-functions however it still leads to a type-mismatch.

@mergify mergify bot removed the ready label Sep 9, 2022
@mergify mergify bot added the ready label Sep 9, 2022
@Borda Borda merged commit bb3e0a4 into devel/classification Sep 12, 2022
@Borda Borda deleted the refactor/classification_10 branch September 12, 2022 08:06
Borda added a commit that referenced this pull request Sep 12, 2022
* example

* stat scores

* revert

* remove old tests

* deprecate binned

* remove init

* warning functionals

* docs update

* add __new__ for class wrapping

* update imports in docstrings

* fix one mistake

* fix doctests

* Apply suggestions from code review

* elif

* kwargs

* kwargs + elif

* text

* fix gpu testing

Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com>
Co-authored-by: Jirka <jirka.borovec@seznam.cz>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Borda added a commit that referenced this pull request Sep 12, 2022
* example

* stat scores

* revert

* remove old tests

* deprecate binned

* remove init

* warning functionals

* docs update

* add __new__ for class wrapping

* update imports in docstrings

* fix one mistake

* fix doctests

* Apply suggestions from code review

* elif

* kwargs

* kwargs + elif

* text

* fix gpu testing

Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com>
Co-authored-by: Jirka <jirka.borovec@seznam.cz>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Borda added a commit that referenced this pull request Sep 13, 2022
* example

* stat scores

* revert

* remove old tests

* deprecate binned

* remove init

* warning functionals

* docs update

* add __new__ for class wrapping

* update imports in docstrings

* fix one mistake

* fix doctests

* Apply suggestions from code review

* elif

* kwargs

* kwargs + elif

* text

* fix gpu testing

Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com>
Co-authored-by: Jirka <jirka.borovec@seznam.cz>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Borda added a commit that referenced this pull request Sep 13, 2022
* example

* stat scores

* revert

* remove old tests

* deprecate binned

* remove init

* warning functionals

* docs update

* add __new__ for class wrapping

* update imports in docstrings

* fix one mistake

* fix doctests

* Apply suggestions from code review

* elif

* kwargs

* kwargs + elif

* text

* fix gpu testing

Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com>
Co-authored-by: Jirka <jirka.borovec@seznam.cz>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready refactoring refactoring and code health
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants