JuliaMolSim / AtomsBase.jl

A Julian abstract interface for atomic structures.
https://juliamolsim.github.io/AtomsBase.jl/
MIT License
82 stars 16 forks source link

Organizing boundary conditions #5

Open Gregstrq opened 2 years ago

Gregstrq commented 2 years ago

Right now, the boundary conditions are specified as AbstractVector{BoundaryCondition}.

I suggest we make it NTuple{D, BoundaryCondition} (D is the dimensionality of the system):

rkurchin commented 2 years ago

I think this makes sense. The last bullet point would also allow checks on dimensionality of positions/velocities. Not sure it makes sense to do that at the top level since people could conceivably want fat or tall matrices, but would be easy to put into a concrete implementation.

Gregstrq commented 2 years ago

I think dimensionality of the system is still a well-defined and clear concept irrespective of whether the users want to stack the coordinates as columns or rows of a matrix.

rkurchin commented 2 years ago

Oh yeah for sure, sorry, I realize now that was unclear. I just meant it doesn't necessarily make sense to have a check on the shape of the matrices coded in at the top level for that reason. I agree with the general proposal!

Gregstrq commented 2 years ago

Sorry, but what do you mean by the matrices coded at the top level? (I thought you were talking about a matrix of all the coordinates or a matrix of all the velocities)

rkurchin commented 2 years ago

Yeah, that is what I meant. I'm just saying since some people may want (e.g. in 3D) 3 x N or N x 3 (or AoS), we don't need to also impose a check that e.g. the first dimension of the positions array is D or something like that at the level of the abstract dispatches, but that may be something that developers of concrete implementations would want, and having the type parameter D accessible will make that easier. I'm agreeing with you, just in a long-winded and confusing way. 😆

rkurchin commented 2 years ago

BC-related comments from @cortner from #23 :

re Periodic...

should this have a flag true/false? Since the bc is implemented as a vector (or tuple) of BCs, you probably want each element of that vector to be e.g. a Periodic() . But now suppose you want a system that is open in the x, y direction and periodic in the z direction (e.g. dislocations). Then you need a different type Open() and you have a type instability. But with Periodic(true), Periodic(false) there is no problem.

Personally, I don't really see how one needs anything other than a boolean here? I can see many other options such as reflecting, embedding, etc but those are not really relevant for the particle system per se, but rather for fields in the background, or for the embedding one would need additional structures anyhow into which to embed the System...

But I don't see any harm in keeping the Periodic type.

cortner commented 4 months ago

also see #97 and #99 -- apologies for duplicating discussions.