DFTK.Model <: AtomsBase.AbstractSystem

[rkurchin]

Resolves #1007

Just wanted to start this here so you could give any feedback early on, also because I've been having trouble getting TestItemRunner working locally and so I'm not currently sure exactly which tests I've broken 🤪, but I'll get that sorted out eventually

The barebones implementation of the interface is here, I have not yet added any additional tests or anything, hence this is still definitely a draft.

[antoine-levitt]

I really don't like the length and getindex overloads, as having them error is very useful for debugging. Looking into this more it really looks like we should have had a geometry field of Model, that would more closely match AtomsBase's System, but model really has more going on than just a geometry, so having model be a subtype of system feels a bit misleading... (that fits with the general "encapsulation over inheritance" stuff)

[mfherbst]

Looking into this more it really looks like we should have had a geometry field of Model, that would more closely match AtomsBase's System

We could just store a system in the Model ? Or simply make a geometry structure right now and use it consistently ?

I think there are also a few subtle details missed in this implementation. E.g. we represent 2D structures by keeping the 3x3 latttice matrix, but conventionally having a tailling zero row and column. In principle this would need to be taken care of as well and respectively the result of n_dimensions etc. adapted.

[antoine-levitt]

We could just store a system in the Model ? Or simply make a geometry structure right now and use it consistently ?

We could and probably should have originally, but now it would be a mess of renaming model.lattice to model.geometry.lattice etc everywhere, for not that much of a benefit...

[rkurchin]

I really don't like the length and getindex overloads

One compromise solution here would be just not to overload those. IIRC, the only things within the interface that actually rely on them is automatically converting between indexed and non-indexed versions of e.g. position, but we could easily just dispatch those (and anything else I'm forgetting) ourselves. The lovely flexibility of interfaces not being rigorously enforced...

I think there are also a few subtle details missed in this implementation. E.g. we represent 2D structures by keeping the 3x3 latttice matrix, but conventionally having a tailling zero row and column. In principle this would need to be taken care of as well and respectively the result of n_dimensions etc. adapted.

Yes, absolutely – I just wanted to get something up to start this conversation, so I definitely didn't handle everything, fully acknowledged, and thanks for pointing out this particular one 😄