Storage and flow states with custom Jacobian + linear solve: the formulation to rule them all#3150
Storage and flow states with custom Jacobian + linear solve: the formulation to rule them all#3150SouthEndMusic wants to merge 89 commits into
Conversation
- Updated the Parameters function to streamline state handling and remove unnecessary variables. - Modified water_balance! to work directly with the full state vector, eliminating the need for reduced state vectors. - Enhanced set_current_basin_properties! to directly use basin storage from the state vector. - Introduced apply_flow_to_basins! to manage flow contributions to basin storage more effectively. - Removed redundant state reduction and flow limit functions, simplifying the flow management logic. - Improved cache handling for flow rates in various node types, ensuring accurate flow tracking. - Updated basin and flow data functions to reflect changes in state handling and removed deprecated variables.
…s massbalance exactly
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
… permanently remove jump opt
…the current cosmetic storage
As it was on main
Removes duplicate keys introduced in merge of main.
|
@visr I did some profiling with the
|
|
That is promising! I'd leave backtracking and threading out of this PR, and mainly check the performance on the full LHM, which I just sent by email. Although there main hardly runs, so we'd compare to the version of Lex' branchbthat we ran with. |
|
There was backtracking in this branch because Lex added it, I removed it again. I did a quick experiment with running the |
|
I've been looking at some LHM profiles. The runtime at the start is really inconsistent over simulated time so I'm not sure what a good benchmark is. I did notice that:
|
fixes #3113.
I did little to no vibe development for this branch, to make things as explainable as possible.
File structure changes
solve.jlwhere the rhs for the ODE lives is renamed toformulate_flows.jlsolve.jlis introduced which contains the custom mass matrix + Jacobian + linear solve implementationDiscontinuity handling
Handling discontinuities of the rhs deserves some attention. Now that I made all positive forcings part of the state, I noticed that the solver isn't able to integrate them exactly, even though they are constant over the timestep. This is because of the memory QNDF uses of previous timesteps, where the forcing is sometimes different. To fix this and related problems, I had to set the
integrator.derivative_discontinuityflag when callbacks change parameters which gets most of the way there, and add astep_limiter!to get there exactly. @Huite this of course relates to our discussion about performance and what you care about to see in the result, but I think I remember discussing with @visr a while back that we should care about preserving the forcing volume.It could be interesting to investigate whether setting
integrator.derivative_discontinuityas a significant impact on performace. If it does, it's worth putting in some more effort to catch also the discontinuities not caused by callbacks but by non-differentiable time interpolations. There are also still hard clamps for certain flows, see #1918.How to interpret the solver config options
The implementations in this PR hook into the solver at a quite low level, so they are less flexible when it comes to certain solver options than we had before. More precisely:
sparsedoesn't mean much for the AD, because the custom implementation represents the Jacobian in a lazy way, and doesn't use sparsity detection and coloring etc. on the rhs. The only sparse arrays still present areJ_innerand the Jacobian of the continuous control compound variables with respect to the storages and flows, and don't think it makes sense to keep the sparse option just for those.autodiff: The way I implemented the Jacobian computation is locked into theForwardDiffAPI. I could refactor it so that it is compatible withDifferentiationInterfaceagain as mentioned here, which requires defining a newCVectortype for all the inputs into a single vector.I think it should be possible to bypass my whole implementation, but we need to think about what we want to support there and how that fits into the config.
Update
I discussed this with @visr and we concluded that:
DifferentiationInterfaceterms. That's a bit more readable and can then make use of theautodiffflag.optimized_solve = truefor this.sparseflag can be removed.The state:$\rightarrow$
CarraysCVectorsThe use of vector components in this implementation is more complex because they are now nested:
(storage, (flows...), pid_integral). This was rather broken in theCArraysimplementation, so I reworked it intoCVectorswhich additional constraints like that the ranges must be contiguous. I also added some pretty printing:The linear solve construction
What I like about the matrix construction in
build_J_inner!is that it very explicitly reveals the structure of the linear solve.PID control
The PID control values are no longer cached, they are computed on-demand in
formulate_pump_and_outlet_flow!.Continuous control
Greedy UserDemand inflow
I think the greedy UserDemand inflow when allocation is not active can be cleaned up a bit: now on main the equal split logic lives both in the flow formulation and in the step limiter, but I think it can be done in a callback where the equal split is assigned to allocated so that other places in the code do not have to know whether it was done by the allocation algorithm or by the simple split.
Latency
As discussed with @visr, precompiling Ribasim takes quite a while because of our custom state vector; a lot of what's precompiled from
OrdinaryDiffEq.jlin their precompile workload is only valid for plain vector states. We could get around this by passing a plain vector state to the solver and only wrapping it for indexing convenience where needed.Allocation
Allocation is outside the scope of this PR, but I noticed some problems with
optimize_multi_objective!while debugging:JuMP.optiomize!, not just after all optimizations are doneallocation_model.objectiveswhere all objectives should live, it's also optimized for for each other objective, not just once at the end.DiscreteControl
The only model that is still misbehaving on this branch is
continuous_concentration_condition(confusing name btw, as it doesn't useContinuousControl). This is a typical 'flipper' model because the controlledLinearResistancedirectly influences the salinity in the Basin being conditioned on. By howDiscreteControlis currently set up (at a per solver timestep basis), how quickly this oscillation happens is fully dependent on the adaptive time stepper, which apparently behaves differently now than with only the flow states. To me the cleanest solution is #2247.