Skip to content

refactor(autogram): Switch to ComputeModuleJacobians#452

Merged
ValerianRey merged 4 commits intomainfrom
accumulate-jacobian-returns-jacobians
Oct 12, 2025
Merged

refactor(autogram): Switch to ComputeModuleJacobians#452
ValerianRey merged 4 commits intomainfrom
accumulate-jacobian-returns-jacobians

Conversation

@PierreQuinton
Copy link
Copy Markdown
Contributor

@PierreQuinton PierreQuinton commented Oct 12, 2025

  • Make AccumulateJacobian return the Jacobian w.r.t. the parameters of the module rather than providing them to the GramianAccumulator directly.
  • Rename the class AccumulateJacobian to ComputeModuleJacobians to better reflect its role.

…f the module rather than providing them to the GramianAccumulator directly.

Rename the class to `ComputeModuleJacobians` to better reflect its role.
@PierreQuinton PierreQuinton added cc: refactor Conventional commit type for any refactoring, not user-facing, and not typing or perf improvements package: autogram labels Oct 12, 2025
@codecov
Copy link
Copy Markdown

codecov bot commented Oct 12, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.

Files with missing lines Coverage Δ
src/torchjd/autogram/_module_hook_manager.py 100.00% <100.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ValerianRey
Copy link
Copy Markdown
Contributor

todos are outdated right?

@ValerianRey
Copy link
Copy Markdown
Contributor

ValerianRey commented Oct 12, 2025

I think this PR should be grouped with the other PRs that you have in mind, so that we can get impactful changes together with this.

@PierreQuinton
Copy link
Copy Markdown
Contributor Author

todos are outdated right?

No, I just put them in the wrong PR.

I think this PR should be grouped with the other PRs that you have in mind, so that we can get impactful changes together with this.

Possibly, but this is a strict improvement that is pretty atomic and independent of anything I have in mind so far (but typically a requirement), it makes it easy to review. Whatever you prefer.

@ValerianRey
Copy link
Copy Markdown
Contributor

todos are outdated right?

No, I just put them in the wrong PR.

I think this PR should be grouped with the other PRs that you have in mind, so that we can get impactful changes together with this.

Possibly, but this is a strict improvement that is pretty atomic and independent of anything I have in mind so far (but typically a requirement), it makes it easy to review. Whatever you prefer.

Yes I understand better now and I agree.

@ValerianRey ValerianRey merged commit 63d9d84 into main Oct 12, 2025
17 checks passed
@ValerianRey ValerianRey deleted the accumulate-jacobian-returns-jacobians branch October 12, 2025 22:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cc: refactor Conventional commit type for any refactoring, not user-facing, and not typing or perf improvements package: autogram

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants