Fix sparsity pattern mismatch and test #2662
Conversation
|
Fails because of missing import. This would require a bump to the minimum in order to pass? |
|
Yes, it will need to use gdalle/SparseMatrixColorings.jl#224, which hasn't been released yet. I jumped the gun on this one a little bit. |
|
Okay, update this PR to do all of that when it's out and then ping me or @oscardssmith for review. I'm going to ignore this for now. |
jClugstor
changed the title
|
Fixes #2653 |
| f.jac(J, duprev, uprev, p, uf.α * uf.invγdt, t) | ||
| # need to do some jank here to account for sparsity pattern of W | ||
| # https://github.com/SciML/OrdinaryDiffEq.jl/issues/2653 | ||
|
|
||
| # we need to set all nzval to a non-zero number | ||
| # otherwise in the following line any zero gets interpreted as a structural zero | ||
| integrator.f.jac_prototype.nzval .= 1.0 | ||
| J .= 1.0 .* integrator.f.jac_prototype | ||
| J.nzval .= 0.0 | ||
| f.jac(J, uprev, p, t) |
There was a problem hiding this comment.
That's not the right Jac call. This is the DAE branch.
There was a problem hiding this comment.
Does the DAE branch not need this as well?
There was a problem hiding this comment.
Oh yes, just a copy paste error
| MM = integrator.f.mass_matrix isa UniformScaling ? integrator.f.mass_matrix(length(integrator.u)) : integrator.f.mass_matrix | ||
| J .= J .+ MM |
There was a problem hiding this comment.
Right, this will cause the mass matrix to be applied twice, won't it.
This might need to go in jacobian2W!? Looks like that might slow things down though...
There was a problem hiding this comment.
jacobian2W! already does this?
There was a problem hiding this comment.
Right, looks like taking away those lines makes it work without applying the MM twice. I thought there would be issues in jacobian2W! if the sparisty pattern of J was still the same as the jac_prototypes, but apparently not.
| end | ||
| end | ||
|
|
||
| # test for https://github.com/SciML/OrdinaryDiffEq.jl/issues/2653#issuecomment-2778430025 |
There was a problem hiding this comment.
I think we can use the simpler MWE here, which does not depend on how SMC handles sparse matrices: #2653 (comment)
Co-authored-by: Christopher Rackauckas <accounts@chrisrackauckas.com>
Co-authored-by: Guillaume Dalle <22795598+gdalle@users.noreply.github.com>
Co-authored-by: Guillaume Dalle <22795598+gdalle@users.noreply.github.com>
|
The failures are internal ITP shifting by one ulp and enzyme. I'll handle those separately. |
3 participants
Checklist
contributor guidelines, in particular the SciML Style Guide and
COLPRAC.
Additional context
Adds test to make sure that the issues brought up in
#2653 (comment) are taken care of.
This will also need to come with a bump to SparseMatrixColorings to a version that's not out yet, I'll need to add the bump when the new version comes out.