Fix HybridJumps and Jumps dependencies

[gzagatti]

This PR attempts to resolve issue #692 in which both the Synapse.jmd and MultivariateHawkes.jmd benchmarks fail to successfully complete after their manifest files were upgraded.

[isaacsas]

Downgrading the OrdinaryDiffEq version isn’t a reasonable fix here. Do we know why both these benchmarks fail on it? We need to fix the failures on the latest versions.

[isaacsas]

Regenerating the jump manifest on 1.9.2 is a good idea though — I hadn’t caught I created it on 1.10… good catch on that one

[gzagatti]

Fortunately, the Hawkes benchmark was only failing because of the mismatched Manifest. Otherwise, it completes successfully. So we can cross this out.

I dug a bit more on the Synapse. It turns out that the upgrade of OrdinaryDiffEq is fine. The problem is the upgrade on IntervalSets. So, I just added IntervalSets from the REPL as

pkg> add IntervalSets@0.73

The moment I tried to add it to Projects.toml in the [compat] session, the benchmark failed with:

┌ Warning: dt(9.505401976639405e-8) <= dtmin(1.1920928955078125e-7) at t=4268.536639826692, and step error estimate = 0.4435074
450063937. Aborting. There is either an error in your model specification or the true solution is unstable.
└ @ SciMLBase ~/.julia/packages/SciMLBase/l4PVV/src/integrator_interface.jl:599

I don't really know what's going on. The version of the Projects.toml and Manifest.toml I just pushed is the only one that worked on my local machine with OrdinaryDiffEq version 6.55.

[isaacsas]

Did you regenerate the manifest after adding [email protected] to the Project.toml too?

[gzagatti]

Yes, I think I did. At least, I typed resolve in the REPL. I also re-run the benchmark to make sure it completed.

[gzagatti]

However, the Manifest resolution changes when you add the version constraint of IntervalSets in [compat]. Maybe, I've misunderstood what this section is supposed to do.

[isaacsas]

Can you check in the HybridJump Project.toml you are generating with the compat bound? Have you made sure you are forcing it to actually use that version in the project, you need a ~ or =, see

https://pkgdocs.julialang.org/v1/compatibility/#Tilde-specifiers

otherwise it will still install the latest version < 1.0 as compatible (at least on CI).

[gzagatti]

Ok, I managed to pin the IntervalSets version with the = directive.

Upgrading IntervalSets will cause the broken compilations below which might be the reason why the benchmark fails with the latest IntervalSets.

┌ Symbolics [0c5d862f-8b57-4792-8d23-62f2024744c7]
│  WARNING: Method definition isapprox(IntervalSets.AbstractInterval{T} where T, IntervalSets.AbstractInterval{T} where T) in module IntervalSets at /home/guilherme/.julia/packages/IntervalSets/6RTOk/src/IntervalSets.jl:144 overwritten in module DomainSets at /home/guilherme/.julia/packages/DomainSets/aafhp/src/domains/interval.jl:52.
│    ** incremental compilation may be fatally broken for this module **
└  
┌ ModelingToolkit [961ee093-0014-501f-94e3-6117800e7a78]
│  WARNING: Method definition isapprox(IntervalSets.AbstractInterval{T} where T, IntervalSets.AbstractInterval{T} where T) in module IntervalSets at /home/guilherme/.julia/packages/IntervalSets/6RTOk/src/IntervalSets.jl:144 overwritten in module DomainSets at /home/guilherme/.julia/packages/DomainSets/aafhp/src/domains/interval.jl:52.
│    ** incremental compilation may be fatally broken for this module **
└  
┌ Catalyst [479239e8-5488-4da2-87a7-35f2df7eef83]
│  WARNING: Method definition isapprox(IntervalSets.AbstractInterval{T} where T, IntervalSets.AbstractInterval{T} where T) in module IntervalSets at /home/guilherme/.julia/packages/IntervalSets/6RTOk/src/IntervalSets.jl:144 overwritten in module DomainSets at /home/guilherme/.julia/packages/DomainSets/aafhp/src/domains/interval.jl:52.
│    ** incremental compilation may be fatally broken for this module **
└  
┌ Synapse [7cc9ea39-daa9-4846-be95-d8a08c9e3c85]
│  WARNING: Method definition isapprox(IntervalSets.AbstractInterval{T} where T, IntervalSets.AbstractInterval{T} where T) in module IntervalSets at /home/guilherme/.julia/packages/IntervalSets/6RTOk/src/IntervalSets.jl:144 overwritten in module DomainSets at /home/guilherme/.julia/packages/DomainSets/aafhp/src/domains/interval.jl:52.
│    ** incremental compilation may be fatally broken for this module **
└  
┌ DomainSets [5b8099bc-c8ec-5219-889f-1d9e522a28bf]
│  WARNING: Method definition isapprox(IntervalSets.AbstractInterval{T} where T, IntervalSets.AbstractInterval{T} where T) in module IntervalSets at /home/guilherme/.julia/packages/IntervalSets/6RTOk/src/IntervalSets.jl:144 overwritten in module DomainSets at /home/guilherme/.julia/packages/DomainSets/aafhp/src/domains/interval.jl:52.
│    ** incremental compilation may be fatally broken for this module **
└  

It is a good idea to port the Synapse benchmark to be self-contained. I just don't have the time right now to do that.

[isaacsas]

The broader issue here is some dependency is holding back using the latest versions of IntervalSets and DomainSets, as I have verified one can install both of them together with no issue.

[isaacsas]

Ideally we would get that dependency to update their Project.toml so the latest versions can be used, which would hopefully fix the issues.

@gzagatti just to confirm, the current code in the PR should now work with the pinned package version?

[gzagatti]

Yes, the current code works with the pinned package version as you can see from the BuildKite artifacts.

I just did a quick check based on your remark. IntervalSets is a dependency of Catalyst.

(Synapse) pkg> why IntervalSets
  Catalyst → ModelingToolkit → DomainSets → IntervalSets
  Catalyst → ModelingToolkit → Symbolics → DomainSets → IntervalSets
  Catalyst → Symbolics → DomainSets → IntervalSets

The Synapse source code pins Catalyst to versions ^10.4.2,11,12, but Catalyst current version is 13.4.1. Perhaps this is the kind of conflict that you mentioned.

I could try to upgrade Catalyst on the Synapse package to see if it runs. This could be a workable solution until we port the code for good.

[isaacsas]

Yes, using such an old Catalyst is likely the issue. It would hold back MTK and Symbolics, which would then likely hold back the offending packages.

[ChrisRackauckas]

Symbolics or ModelingToolkit.

[gzagatti]

I played with the dependencies in the Synapse package but I couldn't get it to compile with the latest Catalyst.

If you're not in a hurry, I can try on my spare time to do port the complete synapse benchmark over here, but it might take a while.

[gzagatti]

I found some time to add a self-contained version of the Synapse benchmark.

The file is quite long. For readability we could potentially add collapsible sections. I haven't done that because I don't know if that is supported and desirable.

I have noticed that memory usage for both PDMP and Coevolve is about 1000x higher (from a few Mb to a few Gb) than in the previous benchmark which was using the Synapse package. I'm not entirely sure why. Execution time is slightly higher but not by very much.

Please let me know where we might need additional explanations, or the code could be simplified. I'm looking forward to your feedback.

[isaacsas]

Is your code copied out of the Synapse repo? Make sure you are giving attribution for that code (it is MIT, but should still get attribution).

[gzagatti]

Yes, I've copied from my branch of the Synapse repo which has a mix of the original code and some modifications that I've made to allow for the use of Coevolve.

I've added at the top of the file the following:

title: Synapse model
author: Yuri Rodrigues, Cian O'Donnel, Romain Veltz, Guilherme Zagatti
weave_options:
    fig_ext: ".png"

But please advise me if there is a better way to attribute the authorship.

[isaacsas]

I would add a comment there that much of this code is adapted from the originally Synapse elife code by (Name et al.), which is available at [link].

Regarding memory use; I haven't looked through everything carefully, but could it be from global variable use? That can cause type instabilities and memory issues in Julia.

[isaacsas]

I mean add a comment and link in the first paragraph.

[ChrisRackauckas]

Yes, const the globals. There's type instabilities here.

[gzagatti]

Attributions have been added.

I've added const to the globals. But It looks like memory used remains high. I hope you can help me find other sources of memory leakage.

[gzagatti]

I have fixed the memory leak using Profile and PProf to locate the leak.

It turns out there was a missing function which is never called by the simulation (so it didn't raise errors) but it caused the leak because every the problem was re-created Julia allocated space for an anonymous function.

Now, the benchmark is fully ported to SciMLBenchmarks.

[isaacsas]

@ChrisRackauckas is this ok with you to merge? I believe @gzagatti feels it is finished.

[gzagatti]

All benchmarks completed successfully and all the assets look correct. It's good to merge on my side.