`remake` doesn't deep copy; mutating new `MTKParameters` affects parent problem as well

jonathanfischer97 commented 2 months ago

`remake` doesn't copy the `MTKParameters` object, unless new parameters are passed directly

If a new problem is made with newprob = remake(odeprob), any mutations to newprob.p will affect odeprob.p because both fields point to the same thing. This side effect is either a bug or not well documented, because a naive user would not expect this.
- This can be checked in this MRE, using code from the docs linked below:
```
using ModelingToolkit
using ModelingToolkit: t_nounits as t, D_nounits as D
```
@parameters α β γ δ @variables x(t) y(t) eqs = [D(x) ~ (α - β y) x D(y) ~ (δ x - γ) y] @mtkbuild odesys = ODESystem(eqs, t)

using OrdinaryDiffEq odeprob = ODEProblem( odesys, [x => 1.0, y => 1.0], (0.0, 10.0), [α => 1.5, β => 1.0, γ => 3.0, δ => 1.0])

newprob = remake(odeprob)

Will evaluate to true

parameter_values(newprob) === parameter_values(odeprob)

This will change odeprob.p even though we are calling replace! on the new parameters

replace!(Tunable(), parameter_values(newprob), rand(4)) odeprob.p
This allows optimizations via remake to possibly be thread unsafe if the problem is remade and then modified within the loop
- not a problem if the new problem is made with new parameters passed directly via remake(odeprob; p = newp)
Perhaps this is by design, but this isn't clearly indicated in the docs here https://docs.sciml.ai/ModelingToolkit/stable/examples/remake/
In fact, the docs at one point encourage a workflow where within the optimization loop, the ODEProblem is copied with remake first, and then the parameters of the new problem modified with replace!:
```
## Optimization loop ...
    newprob = remake(odeprob) # copy the problem with `remake`
    # update the parameter values in-place
    replace!(Tunable(), parameter_values(newprob), x)
```
- This code is modifying odeprob.p in addition to newprob.p every iteration, obviously not threadsafe or generally robust

Solution

If this was by design, then this needs to be indicated in the documentation to notify users' of potential side effects
If not, then remake needs to copy the original problem's parameters when constructing the new problem

Package environment

  [c7e460c6] ArgParse v1.2.0
  [6e4b80f9] BenchmarkTools v1.5.0
⌃ [0f109fa4] BifurcationKit v0.3.6
  [336ed68f] CSV v0.10.14
  [13f3f980] CairoMakie v0.12.11
  [479239e8] Catalyst v14.4.1
  [324d7699] CategoricalArrays v0.10.8
  [aaaa29a8] Clustering v0.15.7
  [35d6a980] ColorSchemes v3.26.0
  [5ae59095] Colors v0.12.11
  [b0b7db55] ComponentArrays v0.15.17
  [f68482b8] Cthulhu v2.15.0
  [a93c6f00] DataFrames v1.6.1
  [1313f7d8] DataFramesMeta v0.15.3
  [85a47980] Dictionaries v0.4.2
  [459566f4] DiffEqCallbacks v3.9.1
  [0c46a032] DifferentialEquations v7.14.0
  [0703355e] DimensionalData v0.28.0
  [b4f34e82] Distances v0.10.11
  [31c24e10] Distributions v0.25.111
  [634d3b9d] DrWatson v2.16.0
  [7c1d4256] DynamicPolynomials v0.6.0
  [06fc5a27] DynamicQuantities v1.0.0
  [61744808] DynamicalSystems v3.3.20
  [86b6b26d] Evolutionary v0.11.2 `~/.julia/dev/Evolutionary`
  [7a1cc6ca] FFTW v1.8.0
  [f6369f11] ForwardDiff v0.10.36
  [e9467ef8] GLMakie v0.10.11
  [f213a82b] HomotopyContinuation v2.11.0
  [5903a43b] Infiltrator v1.8.3
  [b964fa9f] LaTeXStrings v1.3.1
  [2ee39098] LabelledArrays v1.16.0
  [23fbe1c1] Latexify v0.16.5
  [33e6dc65] MKL v0.7.0
  [ee78f7c6] Makie v0.21.11
  [961ee093] ModelingToolkit v9.40.0
  [6f286f6a] MultivariateStats v0.10.3
  [b8a86587] NearestNeighbors v0.4.18
  [8913a72c] NonlinearSolve v3.14.0
  [510215fc] Observables v0.5.5
  [5fb14364] OhMyREPL v0.5.28
  [67456a42] OhMyThreads v0.6.1
  [1dea7af3] OrdinaryDiffEq v6.89.0
  [91a5bcdd] Plots v1.40.8
  [c3e4b0f8] Pluto v0.19.46
  [2dfb63ee] PooledArrays v1.4.3
  [08abe8d2] PrettyTables v2.3.2
  [92933f4c] ProgressMeter v1.10.2
  [1e97bd63] ReachabilityAnalysis v0.26.1
  [295af30f] Revise v3.5.18
  [7e49a35a] RuntimeGeneratedFunctions v0.5.13
  [53ae85a6] SciMLStructures v1.5.0
  [efcf1570] Setfield v1.1.1
  [f1835b91] SpeedMapping v0.3.0
  [90137ffa] StaticArrays v1.9.7
  [2913bbd2] StatsBase v0.34.3
  [f3b207a7] StatsPlots v0.15.7
  [9672c7b4] SteadyStateDiffEq v2.4.0
  [2efcf032] SymbolicIndexingInterface v0.3.30
  [d1185830] SymbolicUtils v3.7.1
  [0c5d862f] Symbolics v6.12.0
  [22787eb5] Term v2.0.6
  [811555cd] ThreadPinning v1.0.2
  [b8865327] UnicodePlots v3.6.4
  [276b4fcb] WGLMakie v0.10.11
  [fdbf4ff8] XLSX v0.10.3
  [8ba89e20] Distributed
  [b77e0a4c] InteractiveUtils
  [37e2e46d] LinearAlgebra
  [9a3f8284] Random
  [10745b16] Statistics v1.10.0

Version info

julia> versioninfo()
Julia Version 1.10.5
Commit 6f3fdf7b362 (2024-08-27 14:19 UTC)
Build Info:
  Official https://julialang.org/ release
Platform Info:
  OS: Linux (x86_64-linux-gnu)
  CPU: 36 × Intel(R) Core(TM) i9-10980XE CPU @ 3.00GHz
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-15.0.7 (ORCJIT, cascadelake)
Threads: 18 default, 0 interactive, 9 GC (on 36 virtual cores)
Environment:
  LD_LIBRARY_PATH = /tmp/.mount_cursorEFTJd7/usr/lib:/tmp/.mount_cursorBSUDMi/usr/lib:/tmp/.mount_cursor0DmoQI/usr/lib:
  JULIA_EDITOR = code
  JULIA_NUM_THREADS = 18

jonathanfischer97 commented 2 months ago

Should note that everything above applies to `odeprob.u0` as well

In threaded optimization loops, I have to remake the problem with newprob = remake(odeprob, p = copy(odeprob.p), u0 = copy(odeprob.u0) in order to avoid multiple threads mutating the same fields of the parent ODEProblem

ChrisRackauckas commented 1 month ago

This is at least the standard behavior with the rest of remake. There is a coming alias system that could improve this behavior. Copy by default could have some major other implications though so it's hard to add into the system (and you cannot assume every p type has a copy so the solve cannot necessarily do it, so ensembleprob on threads has safetycopy for deepcopy, which is a bad fallback).

So it's at least consistent, but I agree I'm not happy with it.

SciML / ModelingToolkit.jl