Fix neighborlist for skewed cells#976
Conversation
src/sisl/geom/_neighbors/_finder.py
Outdated
| ) | ||
|
|
||
| # Required bin size along vector i due to its relation with j | ||
| required_bin_size = 2 * max_R / np.sin(alpha) |
There was a problem hiding this comment.
Ah, I'm just thinking that probably it is not needed to change bin_size to start with 1. We can substitute the 2 here for the bin_size argument for axis i.
Although personally I think it is unnecessary to explain to the users why it starts with 2, and starting with 1 seems simpler to explain/understand. This is something that you added, so up to you to decide :)
There was a problem hiding this comment.
I agree, simpler. Needed another fix in one of the tests.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #976 +/- ##
==========================================
- Coverage 86.67% 86.66% -0.02%
==========================================
Files 412 412
Lines 54375 54368 -7
==========================================
- Hits 47131 47117 -14
- Misses 7244 7251 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This PR fixes a bug that @ialcon brought up for non-orthogonal cells with the neighbor finder.
Essentially, the algorithm assumes that if you are exactly at the center of a bin you can only access atoms within the same bin. This is clearly not the case in skewed cells when not being careful. Essentially, something like this was happening:
@zerothi could you finish the PR (e.g. pre-commit stuff)? I'm abroad on a limited data plan and I don't want to use it to download pre-commit and friends 😄