JuliaMolSim / Molly.jl

Molecular simulation in Julia
Other
371 stars 51 forks source link

System constructor cannot be found? #112

Closed axsk closed 1 year ago

axsk commented 1 year ago

Given a System I want to obtain a new System without mutation. Therefore I thought of using Accessors.jl. However, it turns out that Accessors.set cannot find the constructor of System.

I then tried myself

sys = [... construct some system, e.g. from the example of Molly's README.md]
fields = map(fieldnames(System)) do n getfield(sys, n) end
System(fields...) # ERROR: MethodError: no method matching
System{map(typeof, fields)...}(fields...)  # ERROR: MethodError: no method matching

From what I understand the constructor should be able to be found, right? Can someone confirm this problem? Is this a problem with Molly?

jgreener64 commented 1 year ago

The type parameters of System require some calculation and don't map directly to the fields. This is to allow certain properties to be put into the type domain for dispatch and performance. If you are constructing directly with fields... you would need to specify the type parameters, see: https://github.com/JuliaMolSim/Molly.jl/blob/b172d0d8ca3d199afbc2d4c74b93cae245cb511f/src/types.jl#L391-L394

You can copy a System with deepcopy too. In general, though, I would use the keyword argument constructor for System since it won't be the bottleneck in most code.

Perhaps there is use for a System constructor that takes in an existing System and keyword arguments for any properties that should be changed, returning a copy. At the moment I just do something like:

sys2 = System(
    atoms=sys.atoms,
    coords=sys.coords,
    ...
)
axsk commented 1 year ago

I ended up using exactly that approach and it's fine. A little more verbose then a System(oldsys, coordinates=newcoords) but certainly no priority.

I was just surprised that the 'common' approach did not work (and the type parameter list was too long to spot the problem :D) and Accessors failed too. IMHO in the long run compatibility with Accessors or a 're-constructor' would be nice to have, but I'm fine off knowing about it now.

Thanks for the info!

jgreener64 commented 1 year ago

Perhaps there is use for a System constructor that takes in an existing System and keyword arguments for any properties that should be changed, returning a copy.

This is now on master, e.g. System(sys; coords=(sys.coords .* 0.5)).