Interface and Docs Updates for v0.4.x

[cortner]

Ready for review.

The following functions are now exported:

export bounding_box, 
       periodicity, 
       cell, 
       n_dimensions, 
       species, 
       position, 
       velocity, 
       element, 
       element_symbol, 
       atomic_mass, 
       mass, 
       atomic_number, 
       atomic_symbol, 
       atomkeys, 
       hasatomkey, 
       Atom, 
       FlexibleSystem, 
       FastSystem, 
       isolated_system, 
       periodic_system, 
       PeriodicCell, 
       IsolatedCell 

Changes and amendments to the discussion in #106

• I took an executive decision and renamed particle_species to species which felt more consistent with the rest of the function names, position, mass, velocity.
• I changed how isotopes are managed
• As discussed below, the cell interface is now enforced rather than optional.

[cortner]

Apologies for the delay in pursuing this. It is a lot more complex than I expected and I just didn't have the time. I will try to get as far as possible and ask individuals to contribute.

[cortner]

Reporting some issues that need to be resolved:

• Atom with missing velocity assumes a unit that may be incompatible with everything else. It seems to me this makes no sense. AtomsBase should not be making such assumptions.
• I don't understand why atomic_system, isolated_system and periodic_system are part of atom.jl or even part of AtomsBase. Again there is then a default "FlexibleSystem" called which appears strange to me. Should those functions be moved to AtomsBuilder? Or should AtomsBuilder be integrated into AtomsBase?
• species_type is currently implemented but I don't think this is part of the interface? It returns the type of the particles. I would like to understand what the purpose of this function is before adding it to the interface.
• ascii_plot needs and overhaul. the austrip does weird stuff.
• ChemicalSpecies(:C1) and similar needs to be made to work automagically

[cortner]

This is worth a first look. In my view it is very close to being mergeable if we accept that some bugs will likely remain and can be fixed over time.

CC @mfherbst @rkurchin @jgreener64

[cortner]

Question: Should AtomsBase.Implementation be moved to a separate repository in lib/AtomsBaseImplementation or similarly named?

[cortner]

Note, I've removed the following from the tutorial:

Some property names are reserved and should be considered by all libraries
supporting AtomsBase if possible:

Property name	Unit / Type	Description
:charge	Charge	Total net system charge
:multiplicity	Int (unitless)	Multiplicity of the ground state targeted in the calculation

I see no justification for this, especially the multiplicity which is highly application specific. In addition hiding comments like this at the end of a tutorial is not great. Instead, I started a list of potential future interface functions in Interface part of the docs.

[mfherbst]

Thanks Christoph. I'll take a more detailed look today or tomorrow.

To answer your question from my point of view:
I think lib/AtomsBaseImplementation would be better (separate package). But maybe in light of the discussion in #106 we change the mechanics and naming.

Note, I've removed the following from the tutorial:

This is something I am very unsure about. In the subfield of molecular electronic structure theory, having a total charge and multiplicity on the system (rather than on the atoms) is standard across all codes I know (Gaussian, Orca, Q-Chem, Psi-k, pyscf, ...). This is probably not directly the community we target, but if we want to make it easy to them to integrate with us (or we want to integrate with them) than having this standardised is important.

[cortner]

Then it could become part of the reserved function names for the extended interface. There are a million standards in different different fields and I don't see how we can honour them all. But we can try to agree on such a list.

[cortner]

How about creating a list of recommended names for extended properties in the documentation? (Also intended to discuss future extensions of the interface) Maybe ordered by fields. Then the example can come back into the docs as an example.

[jgreener64]

Broadly looks good, but I'm not sure about the need for the second SystemWithCell abstract type. Couldn't we just require a cell for all systems, or have an open cell default? Alternatively, if it needs to be in the type domain, we could have a second Bool type parameter to AbstractSystem.

[cortner]

Couldn't we just require a cell for all systems

I am ok with that but thought it would be easier for adoption to not require it.

... or have an open cell default?

Well, I think PeriodicCell should be default :)

[jgreener64]

Well, I think PeriodicCell should be default

Either default is fine for me.

[cortner]

Maybe something to point out regarding JG's comments about cells : despite my reservation about adoption I think he is fundamentally correct. Eg a neighbourlist should be a function of the list of positions and the cell. There is a lot of additional information that could be stored in such a cell object - eg reflections, a continuum exterior, etc ...

I also feel that periodicity and bounding box are quite specific to periodic cells.

The question for me is really about making one large redesign or a few smaller steps.

[mfherbst]

Also not too sure about SystemWithCell. Otherwise looks generally ok on my end.

[mfherbst]

Atom with missing velocity assumes a unit that may be incompatible with everything else. It seems to me this makes no sense. AtomsBase should not be making such assumptions.

Agree.

I don't understand why atomic_system, isolated_system and periodic_system are part of atom.jl or even part of AtomsBase. Again there is then a default "FlexibleSystem" called which appears strange to me. Should those functions be moved to AtomsBuilder? Or should AtomsBuilder be integrated into AtomsBase?

This has been done for convenience. I think indeed we should move them to AtomsBuilder now (or replace them by something better ... )

[mfherbst]

species_type is currently implemented but I don't think this is part of the interface? It returns the type of the particles. I would like to understand what the purpose of this function is before adding it to the interface.

I don't remember exactly, but perhaps the idea was to make it possible to easily extract the species type type argument from an AbstractSystem in a generic way. In my code bases it's used nowhere (ExtXYZ, AtomsIO, ASEconvert, AtomsView). Not sure about others, but probably it's better to remove it and only add it back once we need it in the future.

[cortner]

Thank you @mfherbst and @jgreener64 for the feedback. Before proceeding with corrections, there is one big decision to make: should the reorganization of the interface into a "Core" package (whatever the name) and revisiting "AtomsBase" as an implementation of the interface and some tooling around it be done now for 0.4 or can we postpone the reorganization to a 0.5 (after more discussion)

Personally, I would prefer to make 0.4 only about the new interface and reorganize later. But we can of course discuss.

[mfherbst]

Personally, I would prefer to make 0.4 only about the new interface and reorganize later. But we can of course discuss.

I'm ok with that, but if we want to pursue this reorganisation in the future, I would keep exporting the implementation for 0.4 (as mentioned in #106 I think it simplifies version transitions)

[cortner]

Personally, I would prefer to make 0.4 only about the new interface and reorganize later. But we can of course discuss.

I'm ok with that, but if we want to pursue this reorganisation in the future, I would keep exporting the implementation for 0.4 (as mentioned in #106 I think it simplifies version transitions)

Yes, that makes sense.

[rkurchin]

Adding a few small suggestions here. I also don't like adding the SystemWithCell thing, though I do think that opening up to different types of cells could be useful and definitely agree with Christoph's notion of separation (but fundamental interdependence) between coordinates and the cell they're in – as an example, one could also imagine extending this to other coordinate systems (cylindrical, spherical, ...) at some point. Personally, I think every system should have to have a cell anyway so the SystemWithCell thing adds to the namespace without adding much helpful abstraction or functionality.

[cortner]

So my take-away (and this was my next question) is that all three of you prefer to enforce usage of a cell object. This means that cell (currently get_cell) becomes part of the AbstractSystem interface while periodicity and bounding_box become part of the AbstractCell interface.

Regarding naming:

• IsolatedCell seems fairly intuitive. It is a cell that specifies an isolated system.
• PeriodicCell : despite the ambiguity that Rachel pointed out, I think this is a good name. I would argue that Periodic indicates that one can select the periodicity rather than that it is periodic in all coordinate directions. And "open system" is something a bit more general, all it means (IIRC) is that the system interacts with something else outside.

I am ok with this. Just to confirm: Do you all agree with this?

[jgreener64]

Something like this sounds good to me.

[rkurchin]

Seconded!

[cortner]

The latest commits make this ready for final review I think.

Here are a few more comments:

• Re missing velocity discussion : I now implemented a _default_velocity that takes the position unit and guesses a good default. Still not great, but better than `u"bohr/s".
• In the spirit of keeping this a change to the interface only, I suggest to keep isolated_system, periodic_system etc in AtomsBase for now and decide what to do with them (e.g. AtomsBuilder) at the next overhaul where we move the interface into an interface package and start focussing towards reference implementations and good utilities.
• there are a few questions in the discussion above to which I responded but did not make changes. E.g. how isotopes are handled. It can be improved, but I don't think it is critical to merge and register.

[cortner]

@mfherbst -- sorry to bug you. I'm now only waiting for you 👍 to merge and register :)

[mfherbst]

Coverage decreases quite a bit, which could mean we'll find bugs later, but not a blocker for me.