Skip to content

fix: remove padded frames from loss calculation - #166

Merged
roedoejet merged 1 commit into
mainfrom
dev.ap/fs2-loss
Oct 15, 2026
Merged

roedoejet merged 1 commit into
mainfrom
dev.ap/fs2-loss

Conversation

@roedoejet

Copy link
Copy Markdown
Member

@semanticdiff-com

semanticdiff-com Bot commented Sep 16, 2026 •

Copy link
Copy Markdown

Review changes with  SemanticDiff

Changed Files
File Status
  fs2/loss.py  69% smaller
  fs2/tests/test_loss.py  0% smaller

@codecov

codecov Bot commented Sep 16, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 49.89%. Comparing base (b3ee268) to head (e392e22).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #166      +/-   ##
==========================================
+ Coverage   47.92%   49.89%   +1.96%     
==========================================
  Files          25       25              
  Lines        1980     1980              
  Branches      229      229              
==========================================
+ Hits          949      988      +39     
+ Misses        996      950      -46     
- Partials       35       42       +7     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@joanise

joanise commented Sep 16, 2026

Copy link
Copy Markdown
Member

Nothing jumps out at me as problematic. I understand neither * in this context nor masked_select, but you've added unit tests, so presumably those confirm it's doing the right thing.

Do these changes invalidate old models at all, or just improve training by calculating loss better?

Oh, but once again you reached for unittest.TestCase. Stop doing that please.

@roedoejet
roedoejet requested a review from joanise September 16, 2026 17:52
Comment thread fs2/tests/test_loss.py Outdated
Comment on lines +65 to +67
self.assertAlmostEqual(
losses_before[key].item(), losses_after[key].item(), places=4
)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

use

assert losses_before[key].item() == pytest.approx(loses_after[key].item())

places=4 is replaced by either rel= or abs= depending on what makes sense.

See https://docs.pytest.org/en/stable/reference/reference.html#pytest-approx

@joanise joanise left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good, thanks for convert the unit tests.

uses masked select instead of *
@roedoejet
roedoejet merged commit 1b7dfea into main Oct 15, 2026
2 of 4 checks passed
@roedoejet
roedoejet deleted the dev.ap/fs2-loss branch October 15, 2026 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants