Skip to content

test(autogram): Revamp engine tests#418

Merged
ValerianRey merged 21 commits intomainfrom
remove_autojac_dependence_in_autogram_tests
Sep 15, 2025
Merged

test(autogram): Revamp engine tests#418
ValerianRey merged 21 commits intomainfrom
remove_autojac_dependence_in_autogram_tests

Conversation

@PierreQuinton
Copy link
Copy Markdown
Contributor

@PierreQuinton PierreQuinton commented Sep 12, 2025

  • Split test_equivalence_autojac_autogram into test_compute_gramian and test_iwrm_steps_with_autogram
  • Change test_partial_autogram into test_compute_partial_gramian
  • Add compute_gramian_with_autograd and autograd_gramian_forward_backward in forward_backwards.py
  • Compare to autograd gramian instead of autojac in test_compute_gramian and test_compute_partial_gramian
  • Simplify test_autograd_while_modules_are_hooked
  • Add garbage collection step in init functions of speed tests
  • Add speed test for autograd_gramian_forward_backward

…valence_autograd_autogram`. The tests of autogram are not independent from autojac because we still have the full forward_backward phase integration test.
@PierreQuinton PierreQuinton added cc: test Conventional commit type for changes to tests. package: autogram labels Sep 12, 2025
@codecov
Copy link
Copy Markdown

codecov bot commented Sep 12, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.

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

Copy link
Copy Markdown
Contributor

@ValerianRey ValerianRey left a comment

Choose a reason for hiding this comment

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

I think these changes are very good. Just need to address the comments + fix the tolerance for MacOS.

PierreQuinton and others added 9 commits September 12, 2025 16:48
It's a form of backward pass so I think it's fine to have it here + it's one fewer file.
Also reduce batch sizes, otherwise too high cuda memory usage. GPU has lost his will to live after so much time spent rendering graphics for world of warcraft I guess.
@ValerianRey ValerianRey changed the title test: Compare autogram gramian to autograd gramian instead of autojac's test(autogram): Compare to autograd gramian instead of autojac Sep 13, 2025
@ValerianRey
Copy link
Copy Markdown
Contributor

ValerianRey commented Sep 13, 2025

Jokes aside, I think we had a garbage collection issue before b22dcb0. Should be fixed now. Completely unrelated to this PR so I might extract in another one, but flemme.

@PierreQuinton
Copy link
Copy Markdown
Contributor Author

PierreQuinton commented Sep 13, 2025

Flemme LGTM.

Anymore things that come to mind for this PR?

@ValerianRey
Copy link
Copy Markdown
Contributor

Flemme LGTM.

Anymore things that come to mind for this PR?

We need to adjust the tolerances so that tests pass on MacOS

@ValerianRey ValerianRey changed the title test(autogram): Compare to autograd gramian instead of autojac test(autogram): Revamp engine tests Sep 15, 2025
* Move / rename functions
* Improve docstrings
* Reformat
@ValerianRey
Copy link
Copy Markdown
Contributor

@PierreQuinton I Improved a few other things FYI. Merging this

@ValerianRey ValerianRey merged commit d3a9e8b into main Sep 15, 2025
17 checks passed
@ValerianRey ValerianRey deleted the remove_autojac_dependence_in_autogram_tests branch September 15, 2025 13:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cc: test Conventional commit type for changes to tests. package: autogram

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants