-
Notifications
You must be signed in to change notification settings - Fork 5
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
Conversation
CC @sklus |
# todo Add a centralized configuration file | ||
add_definitions(-DGRIDKIT_ENABLE_ENZYME) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this 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.
…y flags in examples.
…nzymePowerElectronicsCheck.
…ecutable CMake macro.
f856ceb
to
88b4153
Compare
@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 upgraded to Enzyme 0.0.182 but the same error persists. :/ |
I am still at v0.0.173 on Frontier. |
I have not done anything to fix it. Trying to setup my dependencies on MacOS to reproduce. |
There was a problem hiding this 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
for (size_t idx = 0; idx < n; ++idx) | ||
{ | ||
v[idx] = 0.0; | ||
} |
There was a problem hiding this comment.
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.
for (size_t idx = 0; idx < x.size(); ++idx) | ||
{ | ||
v[idx] = 0.0; | ||
} |
There was a problem hiding this comment.
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.
I can now confirm that |
There was a problem hiding this 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.
Description
Enzyme Jacobian for
PhasorDynamics::Load
within GridKit. This is done within the model library directly.Proposed changes
src/AutomaticDifferentiation/Enzyme/
to compute the Jacobians. This requires the residual wrapper to be in global namespace, but other definitions can be inside theGridKit::Enzyme
namespace.PhasorDynamics::Load
intoLoadImpl.hpp
. This file in then included inLoad.cpp
(which calls Enzyme and provides a Jacobian) andLoadDependencyTracking.cpp
, such that the two automatic differentiation approaches are compiled in different compilation units.enzyme_jacobian
unit test inLoadTest.hpp
.EnzymeVector
tests more interesting to yield a lower triangular Jacobian rather than a diagonal one.Checklist
-Wall -Wpedantic -Wconversion -Wextra
.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
) toClangEnzymeFlags
withtarget_compile_options
is a functional workaround.