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

Introduce Cell types #99

Open
Tracked by #101
cortner opened this issue May 15, 2024 · 5 comments
Open
Tracked by #101

Introduce Cell types #99

cortner opened this issue May 15, 2024 · 5 comments

Comments

@cortner
Copy link
Member

cortner commented May 15, 2024

This is a proposal based on the zoom call on 15 / 5 / 2024, that replaces #97 .

(1) remove bounding_box and boundary_conditions from the system and replace it with a cell object that encodes the same information.
(2) Implement a type Cell or ParaCell that contains fields cell_vectors and pbc (true / false)
(3) At the same time I would like to replace the AbstractVector with Vector in order to remove that type instability as well.

It would look something like this:

struct FlexibleSystem{D, TPART, TCELL} <: AbstractSystem{D}
    particles::Vector{TPART}
    cell::TCELL
    data::Dict{Symbol, Any}
end

struct ParaCell{D, T}
    cell_vectors::NTuple{D, SVector{3, T}}
    pbc::NTuple{D, Bool}
end

I think this will do two things:

  • make the FlexibleSystem type stable
  • increase flexibility

I'll start on the PR if I get tentative agreement from a few people.

@rkurchin
Copy link
Collaborator

I don't really have particularly strong feelings about how exactly we do things in the FastSystem and FlexibleSystem example implementations, but I presume this has a corresponding proposal for how the interface functions change?

I'm presuming based on our conversation earlier:

  • get rid of these lines entirely
  • change this so that it returns a vector of Bools with a length equal to D?

@cortner
Copy link
Member Author

cortner commented May 16, 2024

I would try to stay as close as possible to the current interface. I don't know exactly what changes are needed until I work on this.

@mfherbst
Copy link
Member

I'm ok with this, provided that we have enough sugar that I don't have to go down to the cell datastructure when I just want to get the cell vectors out of a system. Because that's one thing I really dislike about the packages that abstract away the vectors in a cell structure is that this makes working with them quite inconvenient at times.

@cortner
Copy link
Member Author

cortner commented May 16, 2024

I would try to stay as close as possible to the current interface.

@jgreener64
Copy link
Collaborator

I am okay with this, I don't have strong feelings about how FastSystem and FlexibleSystem are implemented. As Rachel says it is more about the interface functions.

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

No branches or pull requests

4 participants