-
Notifications
You must be signed in to change notification settings - Fork 175
fix: use AbstractVector in jacobian type assertions #1697
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
base: master
Are you sure you want to change the base?
fix: use AbstractVector in jacobian type assertions #1697
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 <[email protected]>
|
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 <[email protected]>
|
@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 |
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