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

Fix test_bertscore_sorting bug + validate idf arg #2727

Merged
merged 6 commits into from
Oct 10, 2024

Conversation

GPPassos
Copy link
Contributor

@GPPassos GPPassos commented Sep 9, 2024

What does this PR do?

Fixes a test related to bert_score sorting, added on: #2347
Additionally, validates the idf input flag to bert_score.

Before submitting
  • Was this discussed/agreed via a Github issue? (no need for typos and docs improvements) (Not discussed, but quick fix)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure to update the docs? (no need IMO)
  • Did you write any new necessary tests?
The test was misconfigured, in a way that `idf` was always passed as tuples, which were coerced into `True`. This PR fixes the test and adds a validation step at `bert_score` that checks if the value of `idf` is an actual boolean.

📚 Documentation preview 📚: https://torchmetrics--2727.org.readthedocs.build/en/2727/

@Borda Borda changed the title Fix test_bertscore_sorting bug + validate idf arg Fix test_bertscore_sorting bug + validate idf arg Sep 10, 2024
@GPPassos
Copy link
Contributor Author

Sorry for the changes in the PR, some commits were meant for a separate branch (at this point, actually related to issue #2728 ). This one should be very basic.

@SkafteNicki SkafteNicki self-assigned this Oct 10, 2024
@SkafteNicki SkafteNicki added this to the v1.4.x milestone Oct 10, 2024
@SkafteNicki SkafteNicki enabled auto-merge (squash) October 10, 2024 08:50
Copy link

codecov bot commented Oct 10, 2024

Codecov Report

Attention: Patch coverage is 33.33333% with 2 lines in your changes missing coverage. Please review.

Project coverage is 39%. Comparing base (6bfb775) to head (c32e25c).
Report is 1 commits behind head on master.

❗ There is a different number of reports uploaded between BASE (6bfb775) and HEAD (c32e25c). Click for more details.

HEAD has 82 uploads less than BASE
Flag BASE (6bfb775) HEAD (c32e25c)
Windows 6 3
python3.8 6 3
cpu 39 20
torch1.13.1+cpu 6 3
macOS 7 4
python3.10 9 5
torch2.0.1 3 2
torch2.4.0+cpu 2 1
python3.11 6 3
torch1.11.0+cpu 2 1
Linux 26 13
python3.9 18 9
torch2.0.1+cpu 6 3
torch1.12.1+cpu 2 1
gpu 3 0
unittest 3 0
torch1.10.2+cpu 2 1
torch1.13.1 2 1
torch2.3.1+cpu 4 2
torch2.2.2+cpu 4 2
torch2.1.2+cpu 2 1
torch2.4.0+cu121 2 1
torch2.4.0 2 1
Additional details and impacted files
@@           Coverage Diff            @@
##           master   #2727     +/-   ##
========================================
- Coverage      69%     39%    -30%     
========================================
  Files         329     316     -13     
  Lines       18083   17921    -162     
========================================
- Hits        12505    7034   -5471     
- Misses       5578   10887   +5309     

@SkafteNicki SkafteNicki merged commit 0fe772d into Lightning-AI:master Oct 10, 2024
61 of 62 checks passed
@mergify mergify bot added the ready label Oct 10, 2024
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.

3 participants