Skip to content
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

Symbolic Jacobian sparsity #1606

Merged
merged 35 commits into from
Aug 19, 2024
Merged

Symbolic Jacobian sparsity #1606

merged 35 commits into from
Aug 19, 2024

Conversation

visr
Copy link
Member

@visr visr commented Jul 3, 2024

This doesn't work yet, but I quickly tried this out yesterday, thought I might as well put it up to look if we should pursue this.
I think the main advantage is that we don't have to maintain our own code for determining the sparsity, which can more easily get out of sync. It does add some dependencies.

The error I get is with DataInterpolations not supporting ::SymbolicUtils.BasicSymbolic{Real}, which looks like this closed issue: SciML/DataInterpolations.jl#168. DataInterpolations has an extension on Symbolics for this purpose, so perhaps we just need to create a minimal example and file an issue.

ERROR: MethodError: no method matching (::DataInterpolations.LinearInterpolation{Vector{…}, Vector{…}, Float64})(::SymbolicUtils.BasicSymbolic{Real})

Closest candidates are:
  (::DataInterpolations.AbstractInterpolation)(::Symbolics.Num)
   @ DataInterpolationsSymbolicsExt C:\Users\visser_mn\.julia\packages\DataInterpolations\NoIUa\ext\DataInterpolationsSymbolicsExt.jl:15
  (::DataInterpolations.AbstractInterpolation)(::Number)
   @ DataInterpolations C:\Users\visser_mn\.julia\packages\DataInterpolations\NoIUa\src\DataInterpolations.jl:22
  (::DataInterpolations.AbstractInterpolation)(::Number, ::Integer)
   @ DataInterpolations C:\Users\visser_mn\.julia\packages\DataInterpolations\NoIUa\src\DataInterpolations.jl:23
  ...

Stacktrace:
  [1] get_area_and_level(basin::Ribasim.Basin{…}, state_idx::Int64, storage::Symbolics.Num)
    @ Ribasim d:\repo\ribasim\Ribasim\core\src\util.jl:57
  [2] set_current_basin_properties!(basin::Ribasim.Basin{…}, storage::SubArray{…})
    @ Ribasim d:\repo\ribasim\Ribasim\core\src\solve.jl:43
  [3] water_balance!(du::ComponentArrays.ComponentVector{…}, u::ComponentArrays.ComponentVector{…}, p::Ribasim.Parameters{…}, t::Float64)

It could be that there are a bunch more issues of this kind once this is resolved.

If it starts to work but we don't want it, we should perhaps use it to test our manual Jacobian sparsity.

@SouthEndMusic
Copy link
Collaborator

SciML/DataInterpolations.jl#310

@SouthEndMusic
Copy link
Collaborator

SouthEndMusic commented Jul 24, 2024

I did some digging. I found out these things:

  • The problem you state above is likely not caused by DataInterpolations.jl, but by SmoothInterpolation.jl, because the problematic type is the output of a function call of that package and the problem goes away when I remove that call. That will be solved by Remove SmoothInterpolation as a dependency #1649
  • Code that branches out based on sparsity-relevant parameters is incompatible, for instance in reduction_factor. I made a non-branching version of it
  • PreallocationTools is not directly compatible with Symbolics, I found a workaround here: https://discourse.julialang.org/t/type-issue-to-get-preallocationtools-diffcache-work-with-symbolics-jacobian-sparsity/88542/3?u=bdekoning
  • To trace trough the code to obtain the jacobian sparsity, the type parameter T in Parameters and its fields must (probably) be Vector{Symbolics.Num}, so the previous point might be irrelevant. I have not yet thought of a non-intrusive way to achieve this

@SouthEndMusic
Copy link
Collaborator

There seems to be a problem with DataInterpolations after all:

SciML/DataInterpolations.jl#317

@SouthEndMusic
Copy link
Collaborator

One thing to keep in mind here is that some basin dependencies are temporary and not 'active' at initialization. To get a proper sparsity pattern, all nodes should be active

Copy link
Member Author

@visr visr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some comments. Let's have a look at the impact performance and binary size.

core/test/utils_test.jl Outdated Show resolved Hide resolved
core/src/util.jl Outdated Show resolved Hide resolved
core/src/util.jl Show resolved Hide resolved
core/src/sparsity.jl Outdated Show resolved Hide resolved
core/src/parameter.jl Outdated Show resolved Hide resolved
core/src/parameter.jl Outdated Show resolved Hide resolved
core/src/Ribasim.jl Outdated Show resolved Hide resolved
@visr
Copy link
Member Author

visr commented Aug 1, 2024

Calculating the Jacobian seems fast, but runtime performance went down a lot. The wrap_forcing function leads to GC and runtime dispatch. One possible way to resolve this that we discussed is to switch the forcing ComponentVector to four separate Vectors. The issue is probably that the ComponentVector does not play well with similar_type used in LazyBufferCache, since the Axis of the components are part of the type.

@visr
Copy link
Member Author

visr commented Aug 1, 2024

We should also have a look at https://github.com/adrhill/SparseConnectivityTracer.jl as an alternative, lighter dependency.

@visr visr mentioned this pull request Aug 5, 2024
@SouthEndMusic
Copy link
Collaborator

SparseConnectivityTracer.jl needs some work: adrhill/SparseConnectivityTracer.jl#145

@SouthEndMusic
Copy link
Collaborator

SparseConnectivityTracer (the unsafe version) now works, only Aqua complains about type piracy on a custom overload, but that should be solved upstream

@SouthEndMusic SouthEndMusic marked this pull request as ready for review August 19, 2024 06:07
Copy link
Member Author

@visr visr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! You'll have to approve yourself ;)

Copy link
Collaborator

@SouthEndMusic SouthEndMusic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@visr visr merged commit 9bdb262 into main Aug 19, 2024
26 of 31 checks passed
@visr visr deleted the symbolic-jacobian-sparsity branch August 19, 2024 11:04
SouthEndMusic added a commit that referenced this pull request Aug 27, 2024
This is a follow-up of #1606.

Hopefully fixes the runtime of the HWS model.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants