fix: use AbstractVector in jacobian type assertions#1697
fix: use AbstractVector in jacobian type assertions#1697ChrisRackauckas-Claude wants to merge 2 commits intoJuliaSymbolics:masterfrom
Conversation
The jacobian function signature accepts any `ops` and `vars`, but the type assertions inside required concrete `Vector` types. When using `SVector` or other `AbstractVector` subtypes, `vec(scalarize(ops))` returns the same type (not a `Vector`), causing a TypeError. This changes `Vector` to `AbstractVector` in the type checks and assertions to properly support all AbstractVector subtypes. Fixes JuliaSymbolics#1691 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1697 +/- ##
===========================================
- Coverage 79.80% 21.94% -57.86%
===========================================
Files 55 55
Lines 5159 5117 -42
===========================================
- Hits 4117 1123 -2994
- Misses 1042 3994 +2952 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Add test case for issue JuliaSymbolics#1691 to ensure jacobian works correctly with SVector and other AbstractVector subtypes. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
@AayushSabharwal why are there so many type assertions in the first place? |
|
Type stability, it helps with the benchmarks quite a bit. |
|
Because unwrap can return numbers? |
|
Because |
|
Why wouldn't scalarize infer? |
|
Given just |
|
Seems like it could be useful if scalarizing gave length 1 vectors? |
|
Not really. |
|
I can (probably) turn scalarize into a different function called |
|
Yeah because if this needs to asset |
|
Any news here? If it takes longer to resolve, a quick fix could be to add a fallback method without the type assertions (and add the |
|
@ChrisRackauckas @AayushSabharwal if I prepared the fix proposed in my previous comment, would you accept it? |
|
Yeah, that works. Thanks! |
|
I tried my proposed fix, but that leads to a method ambiguity and I do not know enough how this library works to resolve it. |
Summary
jacobianwith non-Vectorinputs likeSVectorVectortoAbstractVectorin type checks and assertions in thejacobianfunctionProblem
The
jacobianfunction atsrc/diff.jl:649-656usedVectorin type assertions:However,
vec(scalarize(ops))can return otherAbstractVectorsubtypes (e.g.,SVector), causing aTypeError: in typeassert, expected Vector{Num}, got a value of type SVector{2, Num}.Solution
Replace
VectorwithAbstractVectorin all type checks and assertions within the function to properly support allAbstractVectorsubtypes.Test
Fixes #1691
🤖 Generated with Claude Code