search

JuliaMolSim / Molly.jl

Molecular simulation in Julia
Other
372 stars 51 forks source link

Inconsistency with AtomsBase interface #55

Closed jrdegreeff closed 2 years ago

jrdegreeff commented 2 years ago

There are a couple things that the System implementation is missing for it to be fully interchangable with other AtomsBase implementations.

  1. atomic_symbol(::System, ::Integer) is not implemented
  2. getindex(::System, ::Integer) returns a struct with insufficient fields to be useful in the way that iterating over other AbstractSystem is useful. Specifically this is a problem in my Atomistic and DFTK integration code where I have to have a special case for a System versus an AbstractSystem. This could be resolved simply by having getindex return an AtomView (though any current usage of getindex would have to be modified).
jrdegreeff commented 2 years ago

Looking at the codecov report, the current getindex implementation is not used in any of the tests, so this might be an even easier change than I was expecting. I can put together a quick prototype PR if you'd like.

jgreener64 commented 2 years ago

If you have the time that would be great. I guess atomic_symbol can use the element field in atoms_data.

jrdegreeff commented 2 years ago

I've had a super busy week, but sitting down now to do this. I'll open a PR momentarily.