Skip to content

CompatHelper: bump compat for ImplicitDifferentiation in [weakdeps] to 0.8 for package ControlSystemsBase, (keep existing compat) #998

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

github-actions[bot]
Copy link
Contributor

This pull request changes the compat entry for the ImplicitDifferentiation package from 0.7.2 to 0.7.2, 0.8 for package ControlSystemsBase.
This keeps the compat entries for earlier versions.

Note: I have not tested your package with this new compat entry.
It is your responsibility to make sure that your package tests pass before you merge this pull request.

…o 0.8 for package ControlSystemsBase, (keep existing compat)
@baggepinnen baggepinnen force-pushed the compathelper/new_version/2025-06-12-00-25-24-341-00035705966 branch from 6f7a3d4 to c766ddc Compare June 12, 2025 00:25
@gdalle
Copy link
Contributor

gdalle commented Jun 14, 2025

Your test error should be fixed by JuliaDecisionFocusedLearning/ImplicitDifferentiation.jl#174. I had a bit of a fight with input types in the new release

@gdalle
Copy link
Contributor

gdalle commented Jun 15, 2025

Fix released on my end, wanna close this PR and reopen again?

@baggepinnen baggepinnen reopened this Jun 15, 2025
@JuliaControlBot
Copy link

This is an automated message.
Plots were compared to references. 3/11 images have changed, see differences below.
After pulling this PR, please update the reference images by creating a PR to ControlExamplePlots.jl here.

Difference Reference Image New Image
✔️ 0.0 Reference New
✔️ 0.0 Reference New
✔️ 0.0 Reference New

Copy link

codecov bot commented Jun 15, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 24.86%. Comparing base (cbfe3c9) to head (c766ddc).

Additional details and impacted files
@@             Coverage Diff             @@
##           master     #998       +/-   ##
===========================================
- Coverage   92.52%   24.86%   -67.67%     
===========================================
  Files          41       41               
  Lines        5085     5043       -42     
===========================================
- Hits         4705     1254     -3451     
- Misses        380     3789     +3409     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@gdalle
Copy link
Contributor

gdalle commented Jun 15, 2025

Okay, so what's happening now is:

  • In the update of ImplicitDifferentiation.jl, I bumped DifferentiationInterface.jl from v0.6 to v0.7, which enforces "strict" preparation: the type of inputs during preparation must now be the same as during execution.
  • Iterative solvers from Krylov.jl like working with Vectors rather than whatever custom array type you're using (in this case UpperTriangular).
  • To make this strict check pass, I added a copyto! step from the Vector to your reshaped custom array type before the pullback.
  • This is the step that is erroring because an UpperTriangular is not writable in-place.

@gdalle
Copy link
Contributor

gdalle commented Jun 15, 2025

Could you maybe extract an MWE for me and put it in JuliaDecisionFocusedLearning/ImplicitDifferentiation.jl#176?

@gdalle
Copy link
Contributor

gdalle commented Jun 20, 2025

Gentle bump here for @baggepinnen, an MWE would help me with design decisions on what should be supported or not in ImplicitDifferentiation :)

@baggepinnen
Copy link
Member

Would it help if I convert some triangular matrix to a regular matrix? I don't have much time to dedicate to this extension, it was developed as a technology demonstrator but I have no use cases for it at the moment. It was likely a mistake of mine to rely on such an actively developed package in ControlSystems.jl which is intended to be very robust and stable, but I can't quite remove the support without making a breaking release, which I don't want to do. I'd be willing to accept some loss of performance for reduced maintenance burden and maybe a well-placed Matrix(X) would be sufficient?

@gdalle
Copy link
Contributor

gdalle commented Jun 23, 2025

On the plus side, you're the only external user of the library, so I can easily take your needs into account! I will be using ImplicitDifferentiation for my research a lot more in the near future, hence the recent wave of changes. But it's hard for me to settle on the right design with basically no external feedback, hence the trial and error ^^ sorry about that.

My main design hesitation is whether I should allow arbitrary AbstractArrays to serve as the solution y and conditions c. Understanding your use case for UpperTriangular would be a useful data point for my reflection, even if you cannot extract an MWE for lack of time.

@baggepinnen
Copy link
Member

No hard feelings, the library is accurately labeled as experimental with the 0.x version number, so I take absolutely no issue with you experimenting with your library!

Understanding your use case for UpperTriangular would be a useful data point for my reflection

I assume that UpperTriangular comes into play due to plyapc/plyapd returning UpperTriangular matrices. The solution to the Lyapunov equation is positive definite, and these functions return a Cholesky factor of the solution rather than the solution itself for numerical reasons.

@gdalle
Copy link
Contributor

gdalle commented Jul 1, 2025

Hey @baggepinnen,
I've thought long and hard about this, and my conclusion is that structured matrices like UpperTriangular or SparseMatrixCSC won't play nice with ImplicitDifferentiation anyway. One of the reasons is that linear solvers prefer vectors, so we flatten everything, which tends to forget the underlying structure.
My recommendation would be to express the solver and conditions based on the vector of values above and including the diagonal, and then reconstruct the UpperTriangular after the implicit function has been called. Here's an MWE of what that could look like:

using ImplicitDifferentiation
using LinearAlgebra
using ForwardDiff

function vector_from_uppertriangular(U::UpperTriangular)
    mask = triu(trues(size(U)))
    return U[mask]
end

function uppertriangular_from_vector(v::AbstractVector)
    n = isqrt(2 * length(v))
    M = broadcast(1:n, transpose(1:n)) do i, j
        if i <= j
            return v[(j - 1) * j ÷ 2 + i]
        else
            return zero(eltype(v))
        end
    end
    return UpperTriangular(M)
end

initial_solver(x) = UpperTriangular(sqrt.(x * x'))
initial_conditions(x, U) = UpperTriangular(U .^ 2 .- x * x')

function flat_solver(x)
    U = initial_solver(x)
    y = vector_from_uppertriangular(U)
    return y, nothing
end

function flat_conditions(x, y, _z)
    U = uppertriangular_from_vector(y)
    C = initial_conditions(x, U)
    return vector_from_uppertriangular(C)
end

implicit = ImplicitFunction(flat_solver, flat_conditions)
differentiable_solver(x) = uppertriangular_from_vector(implicit(x)[1])

Let's check that it works:

julia> for n in 1:10
           U = UpperTriangular(rand(n, n))
           @assert uppertriangular_from_vector(vector_from_uppertriangular(U)) == U
       end

julia> x = float.(1:3)
3-element Vector{Float64}:
 1.0
 2.0
 3.0

julia> differentiable_solver(x)
3×3 UpperTriangular{Float64, Matrix{Float64}}:
 1.0  1.41421  1.73205
     2.0      2.44949
             3.0

julia> ForwardDiff.jacobian(differentiable_solver, x)
9×3 Matrix{Float64}:
 1.0       0.0       0.0
 0.0       0.0       0.0
 0.0       0.0       0.0
 0.707107  0.353553  0.0
 0.0       1.0       0.0
 0.0       0.0       0.0
 0.866025  0.0       0.288675
 0.0       0.612372  0.408248
 0.0       0.0       1.0

julia> ForwardDiff.jacobian(differentiable_solver, x)  ForwardDiff.jacobian(initial_solver, x)
true

This should keep working across versions of ImplicitDifferentiation, the main thing that I'm changing at the moment are the keyword arguments

@gdalle
Copy link
Contributor

gdalle commented Jul 2, 2025

Alternately, I have a prototype branch which should be completely agnostic to types here: JuliaDecisionFocusedLearning/ImplicitDifferentiation.jl#180

Can you maybe try it out?

EDIT: it is now on the main branch

@gdalle
Copy link
Contributor

gdalle commented Jul 13, 2025

Gentle bump on this one @baggepinnen, hopefully it should save you the trouble of capping the version of ImplicitDifferentiation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants