Simplify the System API?

[Luthaf]

Opening a meta-issue to discuss possible simplification for the System API. The System is a bit of a God Object, with an ever growing API.

What we we do to simplify it? And what can we do to encapsulate the internals better, while retaining control on performances?

[antoinewdg]

I don't know if this comment belongs here or in #126.
The more I think about it, the more I feel that encapsulating too many things will be a hell to do. For example, if we want to encapsulate access to particle positions, we will have to make 4 different getter functions (ref, ref mut, unsafe ref, unsafe ref mut) and I do not think we want that.

How I would see a new system is:

• a very transparent SystemGeometry (or SystemConfiguration) that contains:
  • the UnitCell
  • the molecules
  • an SOA equivalent of Vec<Particle>
  • the molecule ids
  • how I see it all of these fields may be public
• Interactions the same as before
• and the System object that contains:
  • the SystemGeometry
  • the Interactions
  • the step
  • the external_temperature

There is still some weirdness with the ParticleKinds associations. Am I right saying that this mapping is used only for debugging/logging purposes and when building the system ?
Do we actually need the particle name -> particle kind association after building, or just the reverse one ?

[Luthaf]

how I see it all of these fields may be public

They can not really be public: we need to ensure consistency between molecules and the molecules ids list.

Am I right saying that this mapping is used only for debugging/logging purposes and when building the system ? Do we actually need the particle name -> particle kind association after building, or just the reverse one ?

We mainly use the kind during a simulation, and the name to set the particle kind when adding a new particle. This can happen in two cases: adding new particles between simulation runs, or adding molecules during the run (GCMC is the main culprit here, and maybe Gibbs ensemble simulations too).

So we need the name => kind association to add new particles during a simulation.
We need the kind => name association for the error messages when no interaction is found.

---

Other than that, I agree with you that having all the access functions will be bad, and the separation seems reasonable.

[antoinewdg]

They can not really be public: we need to ensure consistency between molecules and the molecules ids list.

At least the read-only part could be ?

Do we really need to add particles by name when doing that in the middle of a simulation ?

[Luthaf]

Adding new particles (and removing existing particles) is the whole idea of GCMC (the goal is to equilibrate the system at a given chemical potential by changing the particles number). The issue here is that the particle to add are part of the simulation, not the system, so we can not set the particle kind beforehand.

At least the read-only part could be ?

I don't get you here =). How can we ensure that something is read-only if we are making the field public?

[g-bauer]

What should insertion be capable of, i.e. can there be fractional molecules? The selection of what can be inserted into the system is known before the simulation starts, right?

The easiest case would be to have a "blueprint" of every molecule that can be inserted - stored somewhere and then pick it (clone it) and try to insert it. That would only need a Particle::new(name) prior to the simulation run. Within the simulation, we can pick from the array of possible candidates; since adding to the system hands over the particle/molecule, and no names are needed.

[Luthaf]

The selection of what can be inserted into the system is known before the simulation starts, right?

Yes, but different system can associate different particles kind to a given name. Storing blueprint is what I had in mind, but we store them inside the Simulation, not the System. Because we don't know beforehand which system will be used with the simulation, we can not set the particle kind beforehand, and then throw it away.

We could setup the particle kind in Simulation::setup, but this mean we need to keep the name => kind associations until this point as least.

[Luthaf]

And yeah, this sucks when trying to decouple system and interaction =/ I really have no better idea on how to do this.

[g-bauer]

Wild idea: can we provide the list of insertion targets as arguments to the MCMove? Although, we then have to work out how to check if a molecule is present in the system ...

[Luthaf]

Can you expand on that? I don't get it

[g-bauer]

Mhm, my idea would have been to build the particles before the simulation starts. For that, we need system information, i.e. we need to be able to build a particle with a Kind that resides outside the system. Simply put: provide a Particle::new(ParticleKind) and provide a method to add particles to the system that already have a non-default kind.

Does that make sense?

[Luthaf]

Yes, ok =). This would be possible to do, but we still need to associate the simulation and the system for the setup. This is roughly what I was proposing: use the Simulation::setup function to pre-set the particles kind.

[g-bauer]

we still need to associate the simulation and the system

Well, there is no way to get around this, right?

This is roughly what I was proposing: use the Simulation::setup function to pre-set the particles kind.

Yep. I guess the only difference is that the pre-built particles would not have to reside in System beforehand when we add them as argument to the move.

[antoinewdg]

At least the read-only part could be ?

I don't get you here =). How can we ensure that something is read-only if we are making the field public?

Yeah that was not clear in my head. I guess what I was thinking is that we could expose the immutable slices of the SOA directly instead of having to create every getters.

The good thing if we setup the Kinds in the simulation setup is that we will know exactly how many Kinds there is, and consequently would not need a BTreeMap anymore, only a Vec :)

[Luthaf]

Yes, exposing slices (mutable and immutables) would be nice! We just need to prevent insertions and removal in the vector.

Why would we want to have a Vec for the kind already? If it is just for memory locality, we could use something like ordermap =)

[antoinewdg]

Why would we want to have a Vec for the kind already?

It's just that it is simpler than the current BTreeMap, this would make some functions a little simpler like System::composition and ParticleKinds::all_kinds.

I do not know all the places that use Kinds, but performance or memory locality should not be a issue here.

[Luthaf]

Yes, I messed up with the interaction lookup, where we are also using BTreeMap 😄

[antoinewdg]

Yes this BTreeMap is another story. This one could actually also be changed to a 2D array in this case.

[Luthaf]

For pairs and bonds, yes. We'll still need a map (or a sparse tensor) for angles and dihedrals.