Deltares / Ribasim

Water resources modeling
https://ribasim.org/
MIT License
39 stars 5 forks source link

Remove false positives in Jacobian sparsity #1575

Closed SouthEndMusic closed 2 months ago

SouthEndMusic commented 3 months ago

I looked into matrix colouring for Jacobian computation with ForwardDiff.jl (https://youtu.be/bY2VCoxMuo8?si=RzYQz9m_dq7gRG5k) from which it became clear that Jacobian sparsity is very important for performance. The latest adjustments to sparsity.jl in https://github.com/Deltares/Ribasim/pull/1349 removed complexity but added false positives. I propose removing the false positives again with a generic approach:

abstract type AbstractConnectorNode <: AbstractParamererNode end

and make all connector node structs of this abstract type.

function update_jac_prototype!(
    jac_prototype::SparseMatrixCSC{Float64, Int64},
    node::AbstractConnectorNode,
    graph::MetaGraph,
)::Nothing
    ...
end

for all connector nodes. It is assumed that the flow over a connector node always depends on the upstream basin. Whether this flow depends on the downstream basin can (probably) be deduced from whether fractional flow is allowed downstream of the node type.

SouthEndMusic commented 2 months ago

This issue is closed because it is unrealistic to assume that the flow trough a connector node does not depend on both the upstream and the downstream basin, see also https://github.com/Deltares/Ribasim/pull/1577/#issuecomment-2231064189.