Skip to content

refactor(autojac): Factorize jacobian dict creation#609

Merged
ValerianRey merged 4 commits intomainfrom
refactor/autojac-deduplicate
Mar 6, 2026
Merged

refactor(autojac): Factorize jacobian dict creation#609
ValerianRey merged 4 commits intomainfrom
refactor/autojac-deduplicate

Conversation

@PierreQuinton
Copy link
Contributor

@PierreQuinton PierreQuinton commented Mar 5, 2026

This is mostly to test opencode, but I think it is valuable. I asked it to review the autojac package, it suggested that (with a little bit of back and forth between it and me), I like it, but we don't have to accept it.

@PierreQuinton PierreQuinton added the cc: refactor Conventional commit type for any refactoring, not user-facing, and not typing or perf improvements label Mar 5, 2026
@github-actions github-actions bot changed the title refactor(autojac): Deduplicate jacobian dict creation refactor: Deduplicate jacobian dict creation Mar 5, 2026
@github-actions github-actions bot changed the title refactor: Deduplicate jacobian dict creation refactor(autojac): Deduplicate jacobian dict creation Mar 5, 2026
@PierreQuinton
Copy link
Contributor Author

/opencode:Plan Can you review these changes? Anything to report?

@opencode-agent
Copy link
Contributor

opencode-agent bot commented Mar 5, 2026

This is a solid refactoring that cleanly deduplicates code. Here's my review:

Summary

The PR extracts 30+ lines of duplicated jacobian dict creation logic from both _backward.py and _jac.py into a new create_jac_dict function in _utils.py.

Changes Reviewed

_utils.py (new code, lines 106-156):

  • New create_jac_dict function that handles both the identity case (when opt_jacobians=None) and the validation case
  • New check_consistent_first_dimension helper function for validating that all Jacobians have the same first dimension
  • Clean docstrings following project conventions
  • Proper type hints throughout

_backward.py and _jac.py:

  • Removed duplicated inline logic (~30 lines each)
  • Now import and call create_jac_dict instead
  • Simpler, more maintainable code

Tests (test_backward.py, test_jac.py):

  • Updated to import and use create_jac_dict in the _create_transform tests
  • Tests properly validate the extracted function works correctly

Assessment

No issues found - This is a clean deduplication that:

  • Preserves all existing behavior
  • Improves maintainability
  • Has proper validation
  • Includes good test coverage
  • Follows the project's code style

The refactoring is ready to merge.

New%20session%20-%202026-03-05T20%3A42%3A02.573Z
opencode session  |  github run

Copy link
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.

LGTM, nice catch! (CI will break until #611 is merged)

@ValerianRey ValerianRey changed the title refactor(autojac): Deduplicate jacobian dict creation refactor(autojac): Factorize jacobian dict creation Mar 6, 2026
@ValerianRey ValerianRey merged commit 3fb2e03 into main Mar 6, 2026
17 checks passed
@ValerianRey ValerianRey deleted the refactor/autojac-deduplicate branch March 6, 2026 12:10
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