SunnySuite / Sunny.jl

Spin dynamics and generalization to SU(N) coherent states
Other
86 stars 19 forks source link

Clarify usage of `copy` and `deepcopy` #193

Closed kbarros closed 4 months ago

kbarros commented 11 months ago

The Julia stdlib defines Base.copy to perform a shallow copy of a collection type. At the moment, Sunny has a number of Base.copy overloads, but these actually follow deep (not shallow) copy semantics. (Although the precedent is confusing.) Note that we could simply define:

Base.copy(o::Langevin) = deepcopy(o)
Base.copy(o::BinningParameters) = deepcopy(o)
Base.copy(o::BinnedArray) = deepcopy(o)
Base.copy(o::LocalSampler) = deepcopy(o)

Is it good to include these definitions in Sunny, or should we remove them, since people can always use the built-in deepcopy?

I'm unsure, because deepcopy seems to be generally discouraged: https://github.com/JuliaLang/julia/issues/42796#issuecomment-951232853

Maybe we should ask on the Julia forums if it is recommended for packages to implement Base.copy for custom types, even if the implementation just calls deepcopy.

For context, there can be some pretty severe memory unsafety "gotchas" when deepcopy interacts with C pointers, e.g., https://github.com/JuliaMath/FFTW.jl/issues/261.

There is hope, however, that the memory dangers might be mitigated in a future Julia version: https://github.com/JuliaLang/julia/issues/52037

kbarros commented 4 months ago

Because of memory gotchas, let's avoid use of deepcopy in the Sunny codebase. For non-shallow copying, we can follow the convention clone_X. For example, Sunny already has a clone_system.