Another FastSystem constructor

[rkurchin]

I found myself wanting to be able to construct a FastSystem without needing to explicitly build the lists of atomic data when they could be inferred, so I added a constructor that will infer atomic numbers and masses if you only supply the symbols. We could also merge this into the original one by providing default values for the latter two arguments. I am completely fine changing this to that instead, i.e. removing the new one I added and instead modifying lines 15-20 to be something like:

# constructor to fetch the types where numbers/masses are explicitly provided
function FastSystem(box, boundary_conditions, positions, atomic_symbols, atomic_numbers=getproperty.(element.(atomic_symbols), :number), atomic_masses=getproperty.(element.(atomic_symbols), :atomic_mass))
    FastSystem{length(box),eltype(eltype(positions)),eltype(atomic_masses)}(
        box, boundary_conditions, positions, atomic_symbols, atomic_numbers, atomic_masses
    )
end

...but that's also a little clunky to read. 🤷‍♀️

[mfherbst]

...but that's also a little clunky to read.

Not if you add line breaks?

Also why are atomic symbols special. What if I want to supply the atomic numbers instead? What I'm getting at is that we have a different but similar mechanism in the Atom and I think for consistency they both should support the same sort of stuff.

Also a test would be good ;).

[rkurchin]

Not if you add line breaks?

Also why are atomic symbols special. What if I want to supply the atomic numbers instead? What I'm getting at is that we have a different but similar mechanism in the Atom and I think for consistency they both should support the same sort of stuff.

All fair points. Do you think basically duplicating the version we have for Atom (i.e. anything that can index into PeriodicTable.elements) would suffice here? While I could see someone wanting to pass either symbols or atomic numbers and have the others inferred, I kind of doubt they'd ever want to pass only atomic masses...

Also a test would be good ;).

Absolutely, wanted to just stick in a quick implementation to start this discussion, would for sure add this before merging. 😉

[mfherbst]

@rkurchin Super sorry. I completely forgot about this PR. I'll take another look later today.

[mfherbst]

Yes I would essentially duplicate the version from Atom, i.e. just pass in a list of Identifiers (type AbstractVector{<:AtomId}) and then just use those to extract symbol, number and mass automatically to fill the kwargs).

[cortner]

Looking at old PRs. Is this to be revived, or should we just have a discussion at the molssi workshop about constructor and updator interfaces?

[mfherbst]

Is this to be revived, or should we just have a discussion at the molssi workshop about constructor and updator interfaces?

A discussion in line with how "fat" should this package become would be good. So essentially, where do we cut between the implementation here, a showcase for the interface and AtomsBuilder.

[rkurchin]

Relevant to #123