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

Clarify usage of copy and deepcopy #193

Closed
kbarros opened this issue Nov 15, 2023 · 1 comment
Closed

Clarify usage of copy and deepcopy #193

kbarros opened this issue Nov 15, 2023 · 1 comment

Comments

@kbarros
Copy link
Member

kbarros commented Nov 15, 2023

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:
JuliaLang/julia#42796 (comment)

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., JuliaMath/FFTW.jl#261.

There is hope, however, that the memory dangers might be mitigated in a future Julia version:
JuliaLang/julia#52037

kbarros added a commit that referenced this issue Nov 15, 2023
Print an error if non-positive `dt` is detected when constructing an integrator. A negative timesteps can be attained with a mutable update, e.g. `dyn.Δt *= -1`.

Also deprecate `Base.copy(::Langevin)`. We might add it back depending on the resolution to #193. The story here will continue to evolve, but for now it's safe to err on the side of a thinner public API.
@kbarros
Copy link
Member Author

kbarros commented Jun 8, 2024

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.

@kbarros kbarros closed this as completed Jun 8, 2024
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

No branches or pull requests

1 participant