Remove ForwardDiff dependency by klamike · Pull Request #42 · grid-opt-alg-lab/PowerDiff.jl

klamike · 2026-03-30T23:43:18Z

No description provided.

klamike · 2026-04-24T21:49:00Z

I think this is good to go now. You were right @samtalki, some of the switching terms were missing...

samtalki · 2026-05-03T18:07:22Z

paging @djturizo as he mentioned he has previously worked out closed form expresions of the implicit function theorem applied to AC OPF in some of his legacy code https://github.com/djturizo/Shortest-Path-OPF

cameronkhanpour · 2026-05-03T19:05:04Z

+    vars = unflatten_variables(flatten_variables(sol, prob), idx)
+    n, m, k = prob.network.n, prob.network.m, prob.n_gen
+
+    J = spzeros(Float64, kkt_dims(prob), kkt_dims(prob))


Suggestion, non-blocking: this is still assembling a SparseMatrixCSC via many scalar insertions. It is already much faster than the old ForwardDiff path, but the allocation count is still fairly high on larger cases. Longer term, it may be worth assembling (I, J, V) triplets and calling sparse(I, J, V, n, n) once, or caching a fixed sparsity pattern and updating only values.

@cameronkhanpour from my understanding, I think you are right that this is the best practice for building a SparseMatrixCSC. @klamike can you confirm?

yep, I should fix this

What Cameron suggests is the most performant way

cameronkhanpour · 2026-05-03T19:05:05Z

+Return branch-flow bases (with `sw = 1`) and local first derivatives with
+respect to `(va_f, va_t, vm_f, vm_t)`.
+"""
+function _branch_flow_primitives(va, vm, sw, constants, l::Int)


Suggestion, non-blocking: _branch_flow_primitives computes values, first derivatives, and second derivatives unconditionally. Some callers only need values and first derivatives, for example the parameter Jacobian and JVP/VJP paths. Splitting this into a lighter value/gradient helper plus a Hessian-enabled helper would avoid extra work and make caller intent clearer.

Generally agree but should avoid over abstracting unless the isolation has a clear use case

Just split the function in 3, there's no over-abstraction problem

cameronkhanpour · 2026-05-03T19:05:05Z

+    return nothing
+end
+
+function _fill_ac_param_jacobian_sw!(Jp::AbstractMatrix, prob::ACOPFProblem,


Suggestion, non-blocking: the :sw derivative formulas are duplicated across the full parameter Jacobian, single-column helper, and matrix-free JVP/VJP implementations. Since this is the highest-risk part of the analytical path, it would be easier to maintain if a lower-level helper emitted the contribution for one dK/dsw_l column and the full matrix, column, JVP, and VJP paths reused that logic where practical.

Agree with this and, more generally, we should try to isolate analytical derivate formula primitives that can be reused throughout the package. I could be wrong but there appears to be a lot of duplicated logic throughout the diff.

cameronkhanpour · 2026-05-03T19:05:06Z


-"""Pre-extract all AC OPF parameters for ForwardDiff closures."""
+"""Pre-extract all AC OPF parameter/state data needed by the analytical slow path."""
 function _ac_kkt_context(prob::ACOPFProblem)


Suggestion, non-blocking: _ac_kkt_context still extracts several vectors inherited from the previous ForwardDiff closure path. In the analytical JVP/VJP code, most of these are not used. Trimming this context to the values needed by the analytical path would remove some small overhead and make the code easier to audit.

agree; would be nice to avoid

cameronkhanpour · 2026-05-03T19:05:06Z

@@ -44,37 +44,68 @@ bus-type column modifications matching PowerModels' `calc_basic_jacobian_matrix`
 function calc_power_flow_jacobian(state::ACPowerFlowState;
                                  bus_types::Union{Vector{Int}, Nothing}=nothing)
    Y_dense = Matrix(state.Y)


Suggestion, non-blocking: converting state.Y to a dense matrix is simple and fine for the current test cases, but it can become a memory bottleneck for larger sparse networks. If AC PF Jacobian performance matters on larger systems, this could be rewritten to iterate over the sparse nonzeros of Y instead of materializing Matrix(state.Y).

It would be nice to avoid densifying, would this be a difficult fix? @cameronkhanpour @klamike

Confused why dp_dva, dp_dvm, dq_dva, etc. are preallocated as sparse arrays right after densifying G and B.

The whole code logic of this function is dense, although that was already the case before this PR, including the densification of state.Y

In general converting the code logic to leverage sparsity is something that was brought up in issue #20 so it may be more "proper" to address this in a separate PR linking to that issue, but I'll leave it up to y'all if we want to get started with that here.

samtalki

This is an incredibly valuable contribution that will be a core piece of many downstream applications, thank you @klamike . To summarize my (minor) comments overall:

It would be helpful to deduplicate some of the repeated physics logic into consistent derivative primitives, and
there are several places where we may be able to better exploit sparsity and avoid densifying matrices.

samtalki · 2026-05-04T01:12:08Z

@@ -44,37 +44,68 @@ bus-type column modifications matching PowerModels' `calc_basic_jacobian_matrix`
 function calc_power_flow_jacobian(state::ACPowerFlowState;
                                  bus_types::Union{Vector{Int}, Nothing}=nothing)
    Y_dense = Matrix(state.Y)


It would be nice to avoid densifying, would this be a difficult fix? @cameronkhanpour @klamike

samtalki · 2026-05-04T01:47:34Z

+    vars = unflatten_variables(flatten_variables(sol, prob), idx)
+    n, m, k = prob.network.n, prob.network.m, prob.n_gen
+
+    J = spzeros(Float64, kkt_dims(prob), kkt_dims(prob))


@cameronkhanpour from my understanding, I think you are right that this is the best practice for building a SparseMatrixCSC. @klamike can you confirm?

samtalki · 2026-05-04T01:57:06Z


-"""Pre-extract all AC OPF parameters for ForwardDiff closures."""
+"""Pre-extract all AC OPF parameter/state data needed by the analytical slow path."""
 function _ac_kkt_context(prob::ACOPFProblem)


agree; would be nice to avoid

samtalki · 2026-05-04T01:57:53Z

-        return ForwardDiff.gradient(
-            x -> dot(u, _ac_kkt_call(prob, ctx, ctx.sw, NamedTuple{(kw,)}((x,)), fixed_nt)),
-            p0)
+    @inbounds for l in 1:prob.network.m


Is this logic in this loop duplicated elsewhere?

samtalki · 2026-05-04T01:58:29Z

+        return v
    end
+
+    @inbounds for l in 1:prob.network.m


Is this logic in this loop duplicated elsewhere?

samtalki · 2026-05-04T02:06:00Z

+    return nothing
+end
+
+function _fill_ac_param_jacobian_sw!(Jp::AbstractMatrix, prob::ACOPFProblem,


Agree with this and, more generally, we should try to isolate analytical derivate formula primitives that can be reused throughout the package. I could be wrong but there appears to be a lot of duplicated logic throughout the diff.

samtalki · 2026-05-04T02:08:37Z

@@ -547,25 +764,16 @@ function kkt(z::AbstractVector, prob::ACOPFProblem, sw::AbstractVector;

    # Materialize rate_a vector once to avoid repeated isnothing checks
    rate_a = isnothing(fmax) ? T[ref[:branch][l]["rate_a"] for l in 1:m] : fmax


For rate_a what happens if there is an l such that T[ref[:branch][l]["rate_a"] is NaN, nothing, or otherwise degenerate? Does the PowerModels parser already take care of these?

I also have the analogous question for the cost coefficient vectors.

samtalki · 2026-05-04T02:09:15Z

+        _add_symmetric_local_hessian!(J, local_idx, prim.d2q_to_hat, coeff_q_to * sw_l,
+                                      -2 * vars.lam_thermal_to[l] * sw_l^2, prim.dq_to_hat)
+
+        for t in 1:4


wondering if there is any duplicated logic in this file

samtalki · 2026-05-04T02:09:57Z

+Return branch-flow bases (with `sw = 1`) and local first derivatives with
+respect to `(va_f, va_t, vm_f, vm_t)`.
+"""
+function _branch_flow_primitives(va, vm, sw, constants, l::Int)


Generally agree but should avoid over abstracting unless the isolation has a clear use case

samtalki · 2026-05-04T02:10:52Z

-
 """
-Compute the reduced-space Lagrangian L(va, vm, pg, qg; duals, sw).
+Return branch-flow coefficients in the form


The interpretation of "branch flow coefficients" could be more clearly documented.

klamike · 2026-05-04T02:54:49Z

Thanks for the detailed comments @samtalki @cameronkhanpour, I will do some passes next week

djturizo

Left some comments

klamike requested a review from samtalki March 30, 2026 23:43

remove forwarddiff dep

28137ce

klamike force-pushed the mk/ac branch from 52c93a0 to 28137ce Compare April 6, 2026 19:01

Fix switching terms

b458a98

klamike marked this pull request as ready for review April 24, 2026 21:47

cameronkhanpour self-requested a review May 3, 2026 18:01

samtalki requested a review from djturizo May 3, 2026 18:07

cameronkhanpour requested changes May 3, 2026

View reviewed changes

cameronkhanpour mentioned this pull request May 3, 2026

Extensibility to other OPF formulations (SOC, QC, LinDist3Flow, etc.) #8

Open

samtalki reviewed May 4, 2026

View reviewed changes

djturizo reviewed May 4, 2026

View reviewed changes

samtalki mentioned this pull request May 5, 2026

Support AC OPF and AC PF in rectangular coordinates #51

Open

4 tasks

		@@ -547,25 +764,16 @@ function kkt(z::AbstractVector, prob::ACOPFProblem, sw::AbstractVector;

		# Materialize rate_a vector once to avoid repeated isnothing checks
		rate_a = isnothing(fmax) ? T[ref[:branch][l]["rate_a"] for l in 1:m] : fmax

Conversation

klamike commented Mar 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

klamike commented Apr 24, 2026

Uh oh!

samtalki commented May 3, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samtalki left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

klamike commented May 4, 2026

Uh oh!

djturizo left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

klamike commented Mar 30, 2026 •

edited

Loading