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

PairList constructor broken due to change in reinterpret behaviour #19

Open
moradza opened this issue Jan 13, 2022 · 4 comments
Open

PairList constructor broken due to change in reinterpret behaviour #19

moradza opened this issue Jan 13, 2022 · 4 comments

Comments

@moradza
Copy link

moradza commented Jan 13, 2022

I have an array of 3*216 and cubic PBC. Can you help with using this package to get the neighbor list?
NeighbourLists.PairList(arrsol, 1e-9, [2 0 0 ; 0 2 0; 0 0 2], [pbc.L pbc.L pbc.L])
The comment above throws error
typeof(arrsol)
Matrix{Float64} (alias for Array{Float64, 2})

@cortner
Copy link
Member

cortner commented Jan 13, 2022

Hm ... it seems that reinterpret changed its behaviour and nobody ever noticed for this package since we always work with SVectors:

X = rand(3, 216);
NeighbourLists.PairList(X, 1e-2, [1 0 0 ; 0 1 0; 0 0 1], (true, true, true))
# ERROR: MethodError: no method matching reinterpret(::Type{SVector{3, Float64}}, ::Matrix{Float64}, ::Tuple{Int64})
# ... 

I suggest the following workaround:

Xs = reinterpret(SVector{3, Float64}, X)[:]
nlist = NeighbourLists.PairList(Xs, 1e-2, [1 0 0 ; 0 1 0; 0 0 1], (true, true, true))

@cortner
Copy link
Member

cortner commented Jan 13, 2022

I'm not too sure I'll fix this since I am considering dropping support for this package in favour of CellListMap.jl. There seems little point in maintaining two packages that do the same thing. But if you are interested in continuing to use NeighbourLists.jl for the time being then I'll welcome PRs.

@moradza
Copy link
Author

moradza commented Jan 13, 2022

I will take a look into CellListMap.jl. Otherwise, I will see how to work around with NeighbourLists.jl. I am new to Julia and not sure how much I can really contribute. But if you got good PRs to learn and implement hit me up, I am looking to learn and contribute!

@cortner
Copy link
Member

cortner commented Jan 13, 2022

Start with CellListMap.jl, I haven't had extensive time to test it but it seems quite good and fast. I'd probably be careful with highly distorted cells and test carefully that the neighbour list is correct in those cases; it took me a long time to fix all the corner cases in NeighbourLists.jl.

@cortner cortner changed the title usage question PairList constructor broken due to change in reinterpret behaviour Jan 13, 2022
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

2 participants