Skip to content

Enzyme Jacobian for PhasorDynamics::Load within GridKit #131

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

Merged
merged 20 commits into from
Jun 12, 2025

Conversation

nkoukpaizan
Copy link
Collaborator

@nkoukpaizan nkoukpaizan commented Jun 6, 2025

Description

Enzyme Jacobian for PhasorDynamics::Load within GridKit. This is done within the model library directly.

Proposed changes

  • Added a wrapper in src/AutomaticDifferentiation/Enzyme/ to compute the Jacobians. This requires the residual wrapper to be in global namespace, but other definitions can be inside the GridKit::Enzyme namespace.
  • Moved most of the implementation for the PhasorDynamics::Load into LoadImpl.hpp. This file in then included in Load.cpp (which calls Enzyme and provides a Jacobian) and LoadDependencyTracking.cpp, such that the two automatic differentiation approaches are compiled in different compilation units.
  • Added a enzyme_jacobian unit test in LoadTest.hpp.
  • Made the EnzymeVector tests more interesting to yield a lower triangular Jacobian rather than a diagonal one.

Checklist

  • All tests pass.
  • Code compiles cleanly with flags -Wall -Wpedantic -Wconversion -Wextra.
  • The new code follows GridKit™ style guidelines.
  • There are unit tests for the new code.
  • The new code is documented.
  • The feature branch is rebased with respect to the target branch.

Further comments

Second part of #124, after #126.

Draft, because an improved way to compile and link with Enzyme is needed before merging. I am open to suggestions. Maybe register the pass as @ashermancinelli [suggested earlier?](https://github.com//pull/29#discussion_r1893111882) - `ClangEnzymeFlags` (which is easy to link to) does not include all needed flags, e.g., `enzyme-auto-sparsity=1`. It works for the load, but will not work in general. The change to the `EnzymeVector` tests was to make sure I catch this. - I was able to change the `enzyme_add_executable` CMake macro to generate object files and libraries. This works in the `examples`, where we are linking in other GridKit libraries explicitly, but some includes are missing when I try to do this within GridKit itself.

Adding the desired flags (e.g., enzyme-auto-sparsity=1) to ClangEnzymeFlags with target_compile_options is a functional workaround.

@nkoukpaizan nkoukpaizan self-assigned this Jun 6, 2025
@nkoukpaizan nkoukpaizan added help wanted Extra attention is needed development Features/Tools related to development of GridKit, rather than use as a library. automatic differentiation labels Jun 6, 2025
@nkoukpaizan nkoukpaizan changed the title Nicholson/autodiff retake Enzyme Jacobian for PhasorDynamics::Load within GridKit Jun 6, 2025
@pelesh
Copy link
Collaborator

pelesh commented Jun 6, 2025

CC @sklus

@nkoukpaizan nkoukpaizan marked this pull request as ready for review June 7, 2025 16:30
Comment on lines +101 to +107
# todo Add a centralized configuration file
add_definitions(-DGRIDKIT_ENABLE_ENZYME)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure what is the purpose of this definition.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Using it to guard the Jacobian test. See

#ifdef GRIDKIT_ENABLE_ENZYME
.

Copy link
Collaborator

@pelesh pelesh left a comment

Choose a reason for hiding this comment

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

This is great work!

There is one test failing on MacOS with Clang 16:

Test project /Users/55y/src/gridkit/build-clang
    Start 21: EnzymePowerElectronicsCheck
1/1 Test #21: EnzymePowerElectronicsCheck ......***Failed    0.01 sec
Result incorrect at line = 10, column = 11, obtained = -2.06027e-14, reference = 0, difference = 2.06027e-14
Result incorrect at line = 11, column = 10, obtained = 2.06027e-14, reference = -0, difference = 2.06027e-14
Dense matrix: Autodiff Jacobian
-1 -0 -0 -0 -9.4e-05 -0 -0 -0 -0 -0 -0 -0 -0 -0 -0 -0 
0 0 0 0 0 0 0 0 0 0 0 0 0 0 1 0 
0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 1 
-1 -0 -0 -0 -9.4e-05 -0 -0 -0 -0 -0 -0 -0 -0 -0 -0 -0 
0 0 0 0 -31.41 0 0 0 0 0 0 0 0 0 0 0 
0 0 0 0 0 -31.41 0 0 0 0 0 0 0 0 0 0 
-0 -0 -0 -0 -0 -0.0013 -0 -0 -0 -0 -0 -0 -1 -0 -0 -0 
-0 -0 -0 -0 -0 -0 -0 -0 -0 -0 -0 -0 -0 -1 -0 -0 
0 0 0 0 0 -0.00013 420 0 0 0 -1 0 -0.1 -0.015708 0.75 0 
0 0 0 0 0 0 0 420 0 0 0 -1 0.015708 -0.1 0 0.75 
0 0 0 0 0 -1.44444 4.66667e+06 0 1.48148e+07 0 -11185.2 2.06027e-14 -1851.85 -174.533 8333.33 0 
0 0 0 0 0 0 0 4.66667e+06 0 1.48148e+07 -2.06027e-14 -11185.2 174.533 -1851.85 0 8333.33 
0 0 0 0 0 0 0 0 0 0 20000 0 0 314.159 -20000 0 
-0 -0 -0 -0 -0 -0 -0 -0 -0 -0 -0 20000 -314.159 -0 -0 -20000 
0 -2857.14 0 0 0 0 0 0 0 0 0 0 2857.14 0 -85.7143 314.159 
0 0 -2857.14 0 0 0 0 0 0 0 0 0 0 2857.14 -314.159 -85.7143 
Dense matrix: Reference Jacobian
-1 0 0 0 -9.4e-05 0 0 0 0 0 0 0 0 0 0 0 
0 0 0 -0 0 0 0 0 0 0 0 0 0 0 1 -0 
0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 1 
-1 0 0 0 -9.4e-05 0 0 0 0 0 0 0 0 0 0 0 
0 0 0 0 -31.41 0 0 0 0 0 0 0 0 0 0 0 
0 0 0 0 0 -31.41 0 0 0 0 0 0 -0 0 0 -0 
0 0 0 0 0 -0.0013 0 0 0 0 0 0 -1 0 0 0 
0 0 0 0 0 0 0 0 0 0 0 0 0 -1 0 0 
0 0 0 0 0 -0.00013 420 0 0 0 -1 0 -0.1 -0.015708 0.75 0 
0 0 0 0 0 0 0 420 0 0 0 -1 0.015708 -0.1 0 0.75 
0 0 0 0 -0 -1.44444 4.66667e+06 0 1.48148e+07 0 -11185.2 -0 -1851.85 -174.533 8333.33 0 
0 0 0 0 0 0 0 4.66667e+06 0 1.48148e+07 0 -11185.2 174.533 -1851.85 0 8333.33 
0 0 0 0 -0 0 0 0 0 0 20000 0 0 314.159 -20000 0 
0 0 0 0 0 0 0 0 0 0 0 20000 -314.159 0 0 -20000 
0 -2857.14 -0 0 -0 0 0 0 0 0 0 0 2857.14 0 -85.7143 314.159 
0 0 -2857.14 0 0 0 0 0 0 0 0 0 0 2857.14 -314.159 -85.7143 
Status: 2


0% tests passed, 1 tests failed out of 1

Total Test time (real) =   0.02 sec

The following tests FAILED:
         21 - EnzymePowerElectronicsCheck (Failed)
Errors while running CTest

I am not sure what is causing it. Needs to be fixed before the PR is merged.

@nkoukpaizan nkoukpaizan force-pushed the nicholson/autodiff-retake branch from f856ceb to 88b4153 Compare June 10, 2025 16:43
@superwhiskers
Copy link
Collaborator

@pelesh i'm not reproducing this error. has it already been fixed? clang 20 / arch linux / enzyme 0.0.182

@pelesh
Copy link
Collaborator

pelesh commented Jun 10, 2025

@pelesh i'm not reproducing this error. has it already been fixed? clang 20 / arch linux / enzyme 0.0.182

It is still there with clang 16.0.6 from Macports and enzyme 0.0.180. Perhaps this really requires the very latest Enzyme?

@nkoukpaizan, which Enzyme version do you use?

@pelesh
Copy link
Collaborator

pelesh commented Jun 10, 2025

@pelesh i'm not reproducing this error. has it already been fixed? clang 20 / arch linux / enzyme 0.0.182

I upgraded to Enzyme 0.0.182 but the same error persists. :/

@nkoukpaizan
Copy link
Collaborator Author

@pelesh i'm not reproducing this error. has it already been fixed? clang 20 / arch linux / enzyme 0.0.182

It is still there with clang 16.0.6 from Macports and enzyme 0.0.180. Perhaps this really requires the very latest Enzyme?

@nkoukpaizan, which Enzyme version do you use?

I am still at v0.0.173 on Frontier.

@nkoukpaizan
Copy link
Collaborator Author

@pelesh i'm not reproducing this error. has it already been fixed? clang 20 / arch linux / enzyme 0.0.182

I have not done anything to fix it. Trying to setup my dependencies on MacOS to reproduce.

Copy link
Collaborator

@superwhiskers superwhiskers left a comment

Choose a reason for hiding this comment

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

just some minor comments on ways to make things more concise if that's something you'd want to do. i'll leave actual approval to @pelesh

Comment on lines +52 to 55
for (size_t idx = 0; idx < n; ++idx)
{
v[idx] = 0.0;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

are we not able to use std::fill to replace this loop? this just appears to be a zero-fill of the vector.

Comment on lines +64 to 67
for (size_t idx = 0; idx < x.size(); ++idx)
{
v[idx] = 0.0;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

same zero-fill comment above. std::fill could be used here.

@pelesh
Copy link
Collaborator

pelesh commented Jun 11, 2025

@pelesh i'm not reproducing this error. has it already been fixed? clang 20 / arch linux / enzyme 0.0.182

I can now confirm that EnzymePowerElectronicsCheck test failure occurs on arm arch only. I cannot reproduce it on Intel i7 with MacOS /clang 16.0.6/enzyme 0.0.173.

Copy link
Collaborator

@pelesh pelesh left a comment

Choose a reason for hiding this comment

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

The failing test is specific to the arm architecture and may be related to an issue in Enzyme itself. That issue is tracked in #137; it should not delay merging this PR.

@nkoukpaizan nkoukpaizan merged commit 9b92afb into develop Jun 12, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automatic differentiation development Features/Tools related to development of GridKit, rather than use as a library. help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants