Update Julia manifest #2201
Update Julia manifest #2201
Conversation
|
Interesting, there are DimensionMismatch errors coming from SparseMatrixColorings. I suspect that is caused by SciML/OrdinaryDiffEq.jl#2567 landing on top of our #2137? For instance for the |
|
That stack trace though 🥲 |
|
@gdalle do you know what is going on here? |
|
From what I can tell, you're trying to use sparse AD by writing into a sparse Jacobian matrix which has a different number of zeros than the sparsity pattern detected during preparation. I would need an MWE to figure out more. |
|
@gdalle does that mean that false positives in the sparsity detection are not allowed? The sparsity in our models can effectively change, so during sparsity detection we try to capture every possible dependency |
|
No, but it means that you must be consistent in the sparsity patterns of the Jacobian matrices you handle. I'm not sure yet whether this is caused by Ribasim or OrdinaryDiffEq or the combination of both, but what's happening here is:
|
|
EDIT: it's actually really weird that it doesn't error before, because I have a check specifically for the number of zeros to match. So I would say there is a small probability of a bug in SparseMatrixColorings, please provide an MWE. |
|
Isolating a MWE is not so easy, unless you're willing to locally run our pixi workflow for writing test models to disk. Just to be sure: is |
It should be, yes. |
|
Interesting. When I print but when I print |
|
Should there be anything in |
|
The fact that the added nonzeros are on the diagonal makes me think that the issue is SciML/OrdinaryDiffEq.jl#2567 (comment). This is a modification of the sparsity pattern a posteriori, but it will be invisible to the detector, and hence to the subsequent coloring. So I will prepare decompression for a matrix with 26 entries, even though it has 28. |
|
@gdalle for a MWE you could download the Ribasim.run("...\generated_testmodels\pid_control\ribasim.toml") |
|
Feel free to contribute the MWE here: SciML/OrdinaryDiffEq.jl#2653 |
|
Test this patch: SciML/OrdinaryDiffEq.jl#2655 |
|
@SouthEndMusic what do you have as |
|
I get this with your MWE: julia> model.integrator.f.jac.jac_prep |> sparsity_pattern
6×6 SparseArrays.SparseMatrixCSC{Bool, Int64} with 28 stored entries:
1 1 1 1 1 ⋅
1 1 1 1 ⋅ 1
1 1 1 1 ⋅ ⋅
1 1 1 1 ⋅ ⋅
1 1 1 1 1 ⋅
1 1 1 1 ⋅ 1
julia> model.integrator.f.sparsity
6×6 SparseArrays.SparseMatrixCSC{Bool, Int64} with 28 stored entries:
1 1 1 1 1 ⋅
1 1 1 1 ⋅ 1
1 1 1 1 ⋅ ⋅
1 1 1 1 ⋅ ⋅
1 1 1 1 1 ⋅
1 1 1 1 ⋅ 1
julia> model.integrator.f.jac_prototype
6×6 SparseArrays.SparseMatrixCSC{Float64, Int64} with 28 stored entries:
1.0 7.10628e-314 7.10628e-314 0.0 0.0 ⋅
7.10628e-314 1.0 7.10628e-314 0.0 ⋅ 0.0
7.10628e-314 7.10628e-314 1.0 0.0 ⋅ ⋅
7.10607e-314 7.10628e-314 0.0 1.0 ⋅ ⋅
2.35747e-314 7.10628e-314 0.0 0.0 1.0 ⋅
7.1061e-314 7.10628e-314 0.0 0.0 ⋅ 1.0
julia> model.integrator.f.jac.jac_prep |> sparsity_pattern
6×6 SparseArrays.SparseMatrixCSC{Bool, Int64} with 28 stored entries:
1 1 1 1 1 ⋅
1 1 1 1 ⋅ 1
1 1 1 1 ⋅ ⋅
1 1 1 1 ⋅ ⋅
1 1 1 1 1 ⋅
1 1 1 1 ⋅ 1Which does not necessarily mean that these matrices looked that way when they were created. Indeed, OrdinaryDiffEq modifies some of them |
|
I figured out why you're getting the ugly stack trace instead of a nicer error message: you're passing as Line 63 in e878635 Adding a julia> solve!(model)
ERROR: DimensionMismatch: `A` and `S` must have the same sparsity pattern.
Stacktrace:
[1] check_same_pattern
@ ~/Documents/GitHub/Julia/SparseMatrixColorings.jl/src/matrices.jl:83 [inlined]
[2] decompress!
@ ~/Documents/GitHub/Julia/SparseMatrixColorings.jl/src/decompression.jl:359 [inlined]The broader issue is that user-provided in-place Jacobians do not know anything about the |
|
Could you try running it again with the version of SparseMatrixColorings from gdalle/SparseMatrixColorings.jl#224 ? |
|
What do the errors say now? |
|
A great, all tests pass, thanks for the fix. Two of the larger test models are now Unstable. That looks like another issue though. Will have to look into it. |
|
Ok good. SciML folks are still deciding whether they want to yank the new release or include my fix |
|
Well we should just yank OrdinaryDiffEqDifferentiation and then bump the minimum so this patch is required in all released versions. |
|
@visr as it turns out the right fix shouldn't come from SparseMatrixColorings but from OrdinaryDiffEq. It has now been merged into OrdinaryDiffEq#master, can you give that branch a try (with the normal release of SparseMatrixColorings)? |
|
Looking good? |
|
Looks great! Everything is green. With just the SparseMatrixColorings branch we tried in #2201 (comment) we still had some models become unstable, but it looks like that is now fixed as well, so it probably was related. |
|
I'm not sure I know what caused the instabilities, but that's part of why I didn't want to merge my SMC fix: tolerating different sparsity patterns within in-place Jacobians is a recipe for disaster. Much better to fix from within OrdinaryDiffEq as we did! |
|
Indeed, thanks for digging in and ensuring the right problem was fixed! |
We temporarily moved to a branch in #2201, but now that's released, go back to the release.
We temporarily moved to a branch in #2201, but now that's released, go back to the release.
Update the Julia Manifest.toml to get the latest dependencies.
Changed packages
Packages still outdated after update
All package versions