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

Unify preds, target input arguments for text metrics [2of2] cer, ter, wer, mer, rouge, squad #727

Merged
merged 19 commits into from
Jan 13, 2022

Conversation

ashutoshml
Copy link
Contributor

@ashutoshml ashutoshml commented Jan 7, 2022

What does this PR do?

Fixes part of #716

Fixes

  • cer
  • mer
  • ter
  • wer
  • rouge
  • squad

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 🙃
Yes

@codecov
Copy link

codecov bot commented Jan 7, 2022

Codecov Report

Merging #727 (8f94875) into master (77babc9) will increase coverage by 0%.
The diff coverage is 91%.

@@          Coverage Diff          @@
##           master   #727   +/-   ##
=====================================
  Coverage      95%    95%           
=====================================
  Files         170    170           
  Lines        6769   6775    +6     
=====================================
+ Hits         6408   6414    +6     
  Misses        361    361           

@ashutoshml ashutoshml changed the title Update naming conventions for cer, ter, wer, sacrebleu, rouge, squad Update naming conventions for cer, ter, wer, mer, rouge, squad Jan 7, 2022
@ashutoshml ashutoshml marked this pull request as ready for review January 7, 2022 05:53
@Borda Borda added API / design refactoring refactoring and code health labels Jan 7, 2022
@Borda Borda added this to the v0.7 milestone Jan 7, 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.

need to finish deprecation/compatibility 🐰

torchmetrics/functional/text/mer.py Show resolved Hide resolved
@ashutoshml ashutoshml marked this pull request as draft January 7, 2022 08:42
@ashutoshml ashutoshml changed the title Update naming conventions for cer, ter, wer, mer, rouge, squad [WIP] Update naming conventions for cer, ter, wer, mer, rouge, squad Jan 7, 2022
@Borda Borda changed the title [WIP] Update naming conventions for cer, ter, wer, mer, rouge, squad Update naming conventions for cer, ter, wer, mer, rouge, squad Jan 8, 2022
@Borda Borda force-pushed the master branch 3 times, most recently from 3a0f7dc to 317182c Compare January 10, 2022 13:05
@Borda Borda added the Important milestonish label Jan 10, 2022
@Borda Borda changed the title Update naming conventions for cer, ter, wer, mer, rouge, squad Unify preds, target input arguments for text metrics [2of2] cer, ter, wer, mer, rouge, squad Jan 10, 2022
@ashutoshml
Copy link
Contributor Author

ashutoshml commented Jan 11, 2022

doctest seem to be failing. Incase we use pyDeprecate, I will remove those docstrings.

there is no rush to use pyDeprecate, it was just a suggestion... cc: @SkafteNicki

Oh I see, then what would be an ideal way of formatting the docstring for deprecated arguments ?

well, I have a plan to add the docstring enrichment in v0.4 🐰

        predictions:
            .. deprecated:: v0.7
                This argument is deprecated in favor of  `preds` and will be removed in v0.8.
        references:
            .. deprecated:: v0.7
                This argument is deprecated in favor of  `preds` and will be removed in v0.8.

This is failing with docstrings. What is the correct way of writing it? I can't find a way online for sphinx

@Borda
Copy link
Member

Borda commented Jan 11, 2022

This is failing with docstrings. What is the correct way of writing it? I can't find a way online for sphinx

pls check how we did it with PL, probably some spacing is needed (cc: @awaelchli )

        predictions:

            .. deprecated:: v0.7
                This argument is deprecated in favor of  `preds` and will be removed in v0.8.

        references:

            .. deprecated:: v0.7
                This argument is deprecated in favor of  `preds` and will be removed in v0.8.

@ashutoshml
Copy link
Contributor Author

This is failing with docstrings. What is the correct way of writing it? I can't find a way online for sphinx

pls check how we did it with PL, probably some spacing is needed (cc: @awaelchli )

        predictions:

            .. deprecated:: v0.7
                This argument is deprecated in favor of  `preds` and will be removed in v0.8.

        references:

            .. deprecated:: v0.7
                This argument is deprecated in favor of  `preds` and will be removed in v0.8.

Tried this. Still failing. Just to confirm: Adding new line between argument name and .. deprecated:: v0.7 is the only indentation change, right ?

@ashutoshml
Copy link
Contributor Author

ashutoshml commented Jan 12, 2022

For cer, I did this and it seems to be working for now. Let me know if this is the right way

    Args:
        preds: Transcription(s) to score as a string or list of strings
        target: Reference(s) for each speech input as a string or list of strings
    Returns:
        Character error rate score

    .. deprecated:: v0.7
        Args:
            predictions:
                This argument is deprecated in favor of  `preds` and will be removed in v0.8.
            references:
                This argument is deprecated in favor of  `target` and will be removed in v0.8.

And I'll update the rest of the public methods with the same.

@ashutoshml
Copy link
Contributor Author

@Borda Updated the code with all the requested changes. Kindly review

Copy link
Contributor

@stancld stancld left a comment

Choose a reason for hiding this comment

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

MER, SQuAD and TER are new metrics in v0.7 thus there's no need to handle naming changes :]

torchmetrics/functional/text/squad.py Outdated Show resolved Hide resolved
torchmetrics/functional/text/ter.py Outdated Show resolved Hide resolved
torchmetrics/text/mer.py Outdated Show resolved Hide resolved
@Borda
Copy link
Member

Borda commented Jan 12, 2022

hi @ashutoshml @stancld maybe jump on a quick call and sync what is the latest way of deprecation, eventually @stancld can directly edit your PR in his IDE :]

@stancld
Copy link
Contributor

stancld commented Jan 12, 2022

hi @ashutoshml @stancld maybe jump on a quick call and sync what is the latest way of deprecation, eventually @stancld can directly edit your PR in his IDE :]

I'm now leaving but will be back in ~2 hrs if you're available :]

@ashutoshml
Copy link
Contributor Author

hi @ashutoshml @stancld maybe jump on a quick call and sync what is the latest way of deprecation, eventually @stancld can directly edit your PR in his IDE :]

I'm now leaving but will be back in ~2 hrs if you're available :]

@stancld. I might not be available then (10:30 pm here RN). Whenever you're available, please have a quick look at the current docstring and suggest changes, if any. I'll update them whenever I start working tomorrow morning.

@mergify mergify bot removed the has conflicts label Jan 13, 2022
@ashutoshml
Copy link
Contributor Author

@Borda Made all the relevant changes. Kindly review

@Borda Borda requested review from Borda and stancld January 13, 2022 07:30
@mergify mergify bot added the ready label Jan 13, 2022
@Borda Borda merged commit 9094985 into Lightning-AI:master Jan 13, 2022
@ashutoshml ashutoshml deleted the unifyname branch January 13, 2022 08:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API / design Important milestonish ready refactoring refactoring and code health
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants