Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Organizing boundary conditions #5

Open
Tracked by #101
Gregstrq opened this issue Oct 5, 2021 · 7 comments
Open
Tracked by #101

Organizing boundary conditions #5

Gregstrq opened this issue Oct 5, 2021 · 7 comments
Labels
question Further information is requested

Comments

@Gregstrq
Copy link
Collaborator

Gregstrq commented Oct 5, 2021

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):

  • Tuple is covariant in its type parameters: Tuple{Periodic, Periodic, DirichletZero}is a subtype ofNTuple{3, BoundaryCondition}`.
  • Widely used variants of boundary condition, such as Periodic, DirichletZero, are singleton types (they don't have any fields). So, if the boundary conditions are specified as for example Tuple{Periodic,Periodic,DirichletZero}, you don't actually need to store this tuple, you can directly dispatch on its type.
  • If we also define the system as AbstractSystem{D}, than we enforce that the size of the tuple of boundary conditions equals to the dimensionality of the system.
@rkurchin
Copy link
Collaborator

rkurchin commented Oct 5, 2021

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
Copy link
Collaborator Author

Gregstrq commented Oct 5, 2021

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
Copy link
Collaborator

rkurchin commented Oct 5, 2021

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!

@rkurchin rkurchin added the question Further information is requested label Oct 5, 2021
@Gregstrq
Copy link
Collaborator Author

Gregstrq commented Oct 5, 2021

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
Copy link
Collaborator

rkurchin commented Oct 5, 2021

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
Copy link
Collaborator

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
Copy link
Member

cortner commented May 16, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants