Skip to content

Refactor key requirements#286

Merged
ValerianRey merged 33 commits intomainfrom
improve-required-output-keys
Mar 31, 2025
Merged

Refactor key requirements#286
ValerianRey merged 33 commits intomainfrom
improve-required-output-keys

Conversation

@ValerianRey
Copy link
Copy Markdown
Contributor

@ValerianRey ValerianRey commented Mar 31, 2025

  • Add RequirementError
  • Refactor check_and_get_keys into check_keys
  • Update tests accordingly
  • Remove the required_keys parameter from Select, Accumulate, _Matrixify and _Reshape and update their instantiations

Note: originally, an idea was to have a method output_keys taking a set of keys as input and returning the output_keys given the provided input_keys (what check_keys does but without the check), and to have a requirement method taking a set of keys as input and returning a boolean as output (whether the given set of input satisfies the requirement of the transform or not). Even though this structure seems better in terms of single responsibility principle, it has the drawbacks of being inefficient, because the requirement method of a Composition would trigger the computation of the requirement method of the child + the computation of the output_keys method of the child. The number of method calls would thus scale quadratically with the number of transforms composed together. With the current structure, this does not happen, which makes it much more efficient with little-to-no downside.

TODO:
Add tests for new RequirementError messages and update the docstrings accordingly:

  • Composition: test the two ways of failing the check
  • Stack
  • Conjunct
  • _AggregateMatrices: test the failing case
  • Diagonalize: test the failing case
  • Select: test the failing case
  • Differentiate: test the failing case
  • Init: test the failing case

Also:

  • Verify that all ValueError where changed to RequirementError
  • Verify formatting of tests
  • Verify that imports are relative

@ValerianRey ValerianRey added package: autojac cc: refactor Conventional commit type for any refactoring, not user-facing, and not typing or perf improvements labels Mar 31, 2025
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 31, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Files with missing lines Coverage Δ
src/torchjd/autojac/_transform/__init__.py 100.00% <100.00%> (ø)
src/torchjd/autojac/_transform/_differentiate.py 100.00% <100.00%> (ø)
src/torchjd/autojac/_transform/accumulate.py 100.00% <100.00%> (ø)
src/torchjd/autojac/_transform/aggregate.py 100.00% <100.00%> (ø)
src/torchjd/autojac/_transform/base.py 100.00% <100.00%> (ø)
src/torchjd/autojac/_transform/diagonalize.py 100.00% <100.00%> (ø)
src/torchjd/autojac/_transform/init.py 100.00% <100.00%> (ø)
src/torchjd/autojac/_transform/select.py 100.00% <100.00%> (ø)
src/torchjd/autojac/_transform/stack.py 100.00% <100.00%> (ø)
src/torchjd/autojac/backward.py 100.00% <100.00%> (ø)
... and 1 more
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ValerianRey ValerianRey merged commit 6c5559f into main Mar 31, 2025
14 checks passed
@ValerianRey ValerianRey deleted the improve-required-output-keys branch March 31, 2025 15: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: autojac

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants