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

Add support for Neighbor list information #68

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

Joroks
Copy link
Collaborator

@Joroks Joroks commented Sep 4, 2024

I took the liberty to pack each neighbor list entry into a single vector. I think that's easier to handle. Alternatively, we could also have each entry as a pair ìatom => neighbors.

Edit: Actually, I think it makes more sense to use Pairs. This makes looping over each neighbour of iatom way easier. This then also has a pretty clean syntax, because you can do iatom, neighs = list[i] and then loop over neihgs

@vchuravy
Copy link
Member

vchuravy commented Sep 5, 2024

Haven't looked in detail at your code, but with an eye on #28, which has a minimal neighborlist implementation, it is very important that we can access the neighborlist without memory allocations.

I think your implementation does that, but I would also define iterators such that you can say for for neighbor in neighbors(lmp, i) or something similar.

@Joroks
Copy link
Collaborator Author

Joroks commented Sep 5, 2024

My Implementation introduces NeighList and NeighListVec. Both are defined as abstract vectors, so we get the iterator functionality for free, we also get a nice looking Base.show for free. I've also made sure that my code doesn't allocate (I think Ref tecnically heap allocates but afaik this can't be avoided), instead I'm using NeighListVec to unsafe_load from the neighbors pointer and then lazily +1 on the returned index to account for juias 1 based indexing.

The usage looks something like this:

for (iatom, neihgs) in find_pair_neighlist(lmp, "zero")
    for jatom in neighs
        # do something
   end
end

I think It's a good idea to have this as an example as well.

What I could also imagine implementing, is providing a combined iterator over the atom pairs

for (iatom, jatom) in eachpair(find_pair_neighlist(lmp, "zero"))
    # do something
end

I think we should also remove the find_ from the function names. From the users perspective they're just retriving the neighbor list instead of finding the index.

Here's the default Base.show output:

find_pair_neighlist(lmp, "zero")
27-element LAMMPS.NeighList:
  1 => Int32[2, 4, 10]
  2 => Int32[3, 5, 11]
  3 => Int32[28, 6, 12]
  4 => Int32[5, 7, 13]
  5 => Int32[6, 8, 14]
  6 => Int32[30, 9, 15]
  ...

@vchuravy
Copy link
Member

vchuravy commented Sep 5, 2024

Looking at https://docs.julialang.org/en/v1/manual/interfaces/#man-interface-iteration I think you are missing eltype.

@Joroks
Copy link
Collaborator Author

Joroks commented Sep 5, 2024

I'm following https://docs.julialang.org/en/v1/manual/interfaces/#man-interface-array.

my structs are defined like this:

struct NeighListVec <: AbstractVector{Int32}
    numneigh::Int
    neighbors::Ptr{Int32}
end

struct NeighList <: AbstractVector{Pair{Int32, NeighListVec}}
    lmp::LMP
    idx::Int
end

so eltype works properly already.

Edit: I shoud probably move these to the top of the file though

src/LAMMPS.jl Outdated Show resolved Hide resolved
src/LAMMPS.jl Outdated Show resolved Hide resolved
src/LAMMPS.jl Outdated Show resolved Hide resolved
src/LAMMPS.jl Outdated Show resolved Hide resolved
Co-authored-by: Valentin Churavy <[email protected]>
src/LAMMPS.jl Show resolved Hide resolved
@vchuravy
Copy link
Member

vchuravy commented Sep 5, 2024

@Joroks you should be able to create branches in this repository, if you do so the Documenter action will generate a preview.

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

Successfully merging this pull request may close these issues.

2 participants