-
Notifications
You must be signed in to change notification settings - Fork 16
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
AtomView and system getindex #74
Comments
Thanks for your question and sorry for the late reply. I have also spotted the cyclic reference once and that's probably something that should be resolved. I'm not sure how, though as each implementation is sort of convenient for certain scenarios, e.g. #68 added parts of the cyclic thing to AtomsView. Regarding examples: AtomView is used in ExtXYZ and the FastSystem implementation, e.g. https://github.com/libAtoms/ExtXYZ.jl/blob/master/src/atoms.jl#L249. Regarding your question, I think the simplest is you define the function Base.getindex(system::MollySystemType, i, prop::Symbol)
if hasatomkey(system, x)
getproperty(AtomsBase, prop)(system, i) # uses the two-argument version
# or
getproperty(AtomsBase, prop)(system)[i] # uses the one-argument version
else
# throw keyerror
end
end I have to admit that is not particularly nice, but should work. Long-term we should think weather we want to have something like this as the default solution in AtomsBase, such that downstream users do not need to worry so much about this. Feel free to make a PR here if you want, but I would probably first try it in Molly and see it does not cause unexpected complications. |
Unfortunately, this does not work for the Maybe I do not understand the |
Actually the error is on my end here. I completely forgot that there are optional atomskeys when I was suggesting my implementation. Charge is of course one of them. In that case I'd suggest to not use |
No worries, thanks for all your help! |
For documentation, this is what the final solution was: function Base.getindex(system::Union{System, ReplicaSystem}, i, x::Symbol)
atomsbase_keys = (:position, :velocity, :atomic_mass, :atomic_number)
custom_keys = (:charge, )
if hasatomkey(system, x)
if x ∈ atomsbase_keys
return getproperty(AtomsBase, x)(system, i)
elseif x ∈ custom_keys
return getproperty(Molly, x)(system, i)
end
else
throw(KeyError("Key $(x) not present in system."))
end
end |
How am I intended to implement
getindex(v.system, v.index, x)
so thatgetindex(v::AtomView, x::Symbol)
works without callingget()
? If I useget()
there is a cyclic reference and I don't think you can usegetfield()
with an AtomView.Couldn't find a library that uses the AtomView approach to AtomsBase besides Molly. Maybe this is an issue with Molly's implementation, but we do not have arrays for mass, charge, atomic number etc stored as part of the system object so those are not accessible directly via
getfield(v.system,x)[i]
or something along those lines. I can access specific properties by name (e.g.atomic_mass(v.system, 1)
) but not for a generic property passed as a symbol togetindex()
.The text was updated successfully, but these errors were encountered: