Is AbstractElement the best name?

[rkurchin]

It could be somewhat confusing terminology in the case where the identifier isn't actually an element. Would something like AbstractIdentifier or AbstractParticleIdentifier be too vague? Do people have other possibilities to suggest?

[jgreener64]

Agree this one is hard to name and AbstractElement might give the wrong impression. I like AbstractParticleIdentifier best from the options you suggest.

[Gregstrq]

In the current version of the interface, AbstractElement semantically corresponds to the object that stores the physical properties of the element besides the coordinates and velocity. At the same time, as far as I understand, AbstractParticle corresponds to both Element, its coordinates and, possible, velocity. As a result, AbstractParticleIndentifier semantically is something that identifies the particle in the system. But we already have such an object: it is the index of the particle.

Since AbstractElement can be either an identifier (like string or Symbol) or a struct with properties, it does not make sense to rename AbstractElement to AbstractIdentifier. If we want to separate the use of AbstractElement as identifier, it makes sense to create a separate

abstract type AbstractIdentifier <: AbstractElement end

But then, in the end, do we really need a separate AbstractParticle type (which semantically is just a container for AbstractElement, coordinate and possibly velocity)? If we remove it, then we can rename what is now AbstractElement into AbstractParticle and have

abstract type AbstractIndentifier <: AbstractParticle end

[rkurchin]

In the current version of the interface, AbstractElement semantically corresponds to the object that stores the physical properties of the element besides the coordinates and velocity. At the same time, as far as I understand, AbstractParticle corresponds to both Element, its coordinates and, possible, velocity. As a result, AbstractParticleIndentifier semantically is something that identifies the particle in the system. But we already have such an object: it is the index of the particle.

I agree with the redundancy you point out here and think perhaps defining the getter functions for the atomic information on Element may not make the most sense, but I disagree that the index of the particle serves the purpose that this object is intended to serve. Every particle in a system would have a unique index, while not every particle would generally have a unique identifier. Think of the case where the identifier is a chemical element. More likely than not there are many particles in the system, but probably only a few chemical elements represented. The identifier allows both grouping according to this type, and also can serve as an "index" a library of simulation parameters. In the case of DFT, this would be things like pseudopotentials for the various elements. For MD, it would be parameters of interatomic potentials (and the identifier might here be a molecule instead). Etc. etc.

Hopefully this helps clarify the distinction between the two.

[Gregstrq]

In that passage, I wanted to say that AbstractParticleIdentifier is a bad name. Because AbstractParticle contains coordinates and velocities, semantically AbstractParticleIdentifier implies the unique index of the particle in the system. And it is certainly not the type of identifier you are talking about.

Rachel, what do you think about removing an AbstractParticle type from the interface (I mean remove a dedicated type which functions as a container of Element, coordinate and possibly velocity)? It could help with naming as well.

[rkurchin]

I don't think AbstractParticleIdentifier implies the unique index. It is totally separate from the index. Imagine I'm simulating a NaCl system that has 8 Na's and 8 Cl's. Particles 1-8 will all have an identifier associated with Na, and 9-16 with Cl.

I could possibly be persuaded on the removing AbstractParticle thing, partially because of the discussion happening here (and in the associated PR). I think it may make some sense not to have a separate type for an individual particle, and it could suffice for getindex(::AbstractSystem) just to return another AbstractSystem object. This also allows more analogous behavior between passing a single index vs. a range of indices. I suspect @mfherbst will have thoughts on this, and also this maybe seems like a discussion for a separate issue at this point...

[Gregstrq]

I don't think AbstractParticleIdentifier implies the unique index. It is totally separate from the index. Imagine I'm simulating a NaCl system that has 8 Na's and 8 Cl's. Particles 1-8 will all have an identifier associated with Na, and 9-16 with Cl.

Yes, but since AbstractParticle is supposed to contain coordinates, Na atoms with different coordinates are different Particle-s but the same Element. And the name AbstractParticleIdentifier sounds like something that identifies AbstractParticle, not an AbstractElement.

I suspect @mfherbst will have thoughts on this, and also this maybe seems like a discussion for a separate issue at this point...

I will create a separate issue then.

[mfherbst]

Having read this discussion, I tend to agree with @Gregstrq that AbstractParticleIdentifier implies something unique across the simulated system. How about AbstractParticleClass?

[rkurchin]

AbstractParticleClass seems too similar-sounding to AbstractElement to me...

Just to throw another one in the hat, which I'm not too attached to...AbstractMoiety?

[rkurchin]

From other discussions on #7...what about AbstractSpecies?

[jgreener64]

I like AbstractSpecies.

[Gregstrq]

Yes, I like this name.

By the way, it would be really nice to be able to use a Symbol in place of AbstractSpecies sometimes.

• One possible option is to define

abstract type AbstractSpecies end

const GeneralSpecies = Union{Symbol, AbstractSpecies}

Then we can put ET<:GeneralSpecies everywhere we have species type parameter.

• Another option is to create

struct SpeciesIdentifier <: AbstractSpecies
   id::Symbol
end

[mfherbst]

Me too, I like the name. I made bad experiences with Unionising like you propose @Gregstrq. The problem is that that puts a symbol (which is gereneric and may have many use cases) on the same level as a very specific custom type. Once you add many methods for GeneralSpecies than you might end up accidentially providing polluting methods for Symbol as well. So I personally prefer the second option.

Hmmm to some extend this also points to the problem raised in #7. Maybe we are overcomplicating things here.

[rkurchin]

Given #23 is getting rid of this type altogether, I'm going to close this for now.