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] Scaling of empirical/MC Fisher for output with more than two dimensions #109

Merged
merged 15 commits into from
May 5, 2024

Conversation

runame
Copy link
Collaborator

@runame runame commented May 1, 2024

Resolves #108.

@f-dangel and me will investigate different definitions for the Fisher in these cases, but for now we will stick to the here implemented convention.

@runame runame added the bug Something isn't working label May 1, 2024
@runame runame requested a review from f-dangel May 1, 2024 23:06
@runame runame self-assigned this May 1, 2024
@coveralls
Copy link

coveralls commented May 1, 2024

Pull Request Test Coverage Report for Build 8961022604

Details

  • 45 of 53 (84.91%) changed or added relevant lines in 4 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.2%) to 88.456%

Changes Missing Coverage Covered Lines Changed/Added Lines %
curvlinops/gradient_moments.py 14 15 93.33%
curvlinops/fisher.py 0 7 0.0%
Files with Coverage Reduction New Missed Lines %
curvlinops/fisher.py 1 26.09%
Totals Coverage Status
Change from base Build 8840487861: -0.2%
Covered Lines: 1249
Relevant Lines: 1412

💛 - Coveralls

Comment on lines -48 to +63
"model_func": lambda: Sequential(Linear(10, 5), ReLU(), Linear(5, 2)),
"model_func": lambda: Sequential(Linear(10, 5), ReLU(), Linear(5, 3)),
"loss_func": lambda: CrossEntropyLoss(reduction="mean"),
"data": lambda: [
(rand(3, 10), classification_targets((3,), 2)),
(rand(4, 10), classification_targets((4,), 2)),
(rand(3, 10), classification_targets((3,), 3)),
(rand(4, 10), classification_targets((4,), 3)),
],
"seed": 0,
},
# same as above, but uses reduction='sum'
{
"model_func": lambda: Sequential(Linear(10, 5), ReLU(), Linear(5, 2)),
"model_func": lambda: Sequential(Linear(10, 5), ReLU(), Linear(5, 3)),
"loss_func": lambda: CrossEntropyLoss(reduction="sum"),
"data": lambda: [
(rand(3, 10), classification_targets((3,), 2)),
(rand(4, 10), classification_targets((4,), 2)),
(rand(3, 10), classification_targets((3,), 3)),
(rand(4, 10), classification_targets((4,), 3)),
Copy link
Owner

Choose a reason for hiding this comment

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

Any reason why these cases changed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not really, I think I simply preferred having a test case with more than 2 classes, but didn't have a particular failure mode in mind.

@runame runame merged commit d43e884 into main May 5, 2024
13 checks passed
@runame runame deleted the fix-scale branch May 5, 2024 21:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Scaling of (KFAC) empirical/MC Fisher broken in some cases when using mean reduction for loss
3 participants