Store and re-use computed neighbor vectors.

[bdice]

Description

I am dealing with an unusual case with Voronoi neighbors in a sparse system (only a few particles), where particles can be their own neighbors or share multiple bonds with the same neighbor. For this case, it's insufficient to identify neighbors by their distance. Instead, I need the actual vectors computed by the Voronoi tessellation.

• Make vec3, vec2, quat constexpr-qualified (also constexpr implies inline).
• Add vectors to NeighborBond, NeighborList.
• Store neighbor vectors in all neighbor bonds, use this data instead of re-computing bond vectors from the query points and points.
• Ignore E402 because flake8 doesn't like some changes made by isort; I prefer isort so I'm ignoring the flake8 warning.

There is an API break worth mentioning here: NeighborList.from_arrays can no longer accept distances, and instead requires vectors. (The distances are computed internally to match the vectors.) I could not think of a way to avoid this API break.

Motivation and Context

In this PR, I re-work the NeighborBond and related classes to store the actual vector for each bond. This vector can be directly re-used in many analysis methods, avoiding the need to perform computing box.wrap(points[point_index] - query_points[query_point_index]). I have benchmarks below. This should help improve correctness in a few edge cases like the Voronoi case I mentioned above, and should make it easier if we ever choose to let freud find neighbors beyond the nearest periodic image. I also caught a bug in the tests because of this change.

How Has This Been Tested?

Existing tests pass with minor tweaks.

Performance:

• RDF is basically unchanged
• BondOrder is ~10% faster
• PMFT is ~10% slower

Overall I'm not concerned about the performance impact of this PR.

Types of changes

• [ ] Bug fix (non-breaking change which fixes an issue)
• [x] New feature (non-breaking change which adds or improves functionality)
• [x] Breaking change (fix or feature that would cause existing functionality to change)
• [ ] Documentation improvement (updates to user guides, docstrings, or developer docs)

Checklist:

• [x] I have read the CONTRIBUTING document.
• [x] My code follows the code style of this project.
• [x] I have updated the documentation (if relevant).
• [x] I have added tests that cover my changes (if relevant).
• [x] All new and existing tests passed.
• [ ] I have updated the credits.
• [ ] I have updated the Changelog.

[codecov[bot]]

Codecov Report

Merging #738 (f22e3f4) into master (9bb343c) will decrease coverage by 0.38%. The diff coverage is 71.42%.

:exclamation: Current head f22e3f4 differs from pull request most recent head 559469d. Consider uploading reports for the commit 559469d to get more accurate results

@@            Coverage Diff             @@
##           master     #738      +/-   ##
==========================================
- Coverage   55.68%   55.29%   -0.39%     
==========================================
  Files          16       16              
  Lines        2462     2407      -55     
  Branches       34       35       +1     
==========================================
- Hits         1371     1331      -40     
+ Misses       1077     1063      -14     
+ Partials       14       13       -1     

Impacted Files	Coverage Δ
freud/locality.pyx	39.36% <71.42%> (-0.05%)	:arrow_down:
freud/util.pyx	32.22% <0.00%> (-3.87%)	:arrow_down:
freud/box.pyx	60.89% <0.00%> (-2.14%)	:arrow_down:
freud/order.pyx	43.72% <0.00%> (-1.33%)	:arrow_down:
freud/diffraction.pyx	71.53% <0.00%> (-0.11%)	:arrow_down:
freud/plot.py	83.48% <0.00%> (+0.97%)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 9bb343c...559469d. Read the comment docs.

[vyasr]

@bdice I don't mind the API break, I think that it's probably reasonable for us at this stage. However, I think that since the 1.0 release the project has been much more consistent in adhering to best practices overall, which includes versioning. I don't have time to review this right now, I will get to it soon, but if we can't find a way to avoid the API break we should stick to semantic versioning and require a 3.0 version. I don't think that's a huge issue; we shouldn't be scared of a major version bump if that's the best way to convey the breakage.

[bdice]

@bdice I don't mind the API break, I think that it's probably reasonable for us at this stage. However, I think that since the 1.0 release the project has been much more consistent in adhering to best practices overall, which includes versioning. I don't have time to review this right now, I will get to it soon, but if we can't find a way to avoid the API break we should stick to semantic versioning and require a 3.0 version. I don't think that's a huge issue; we shouldn't be scared of a major version bump if that's the best way to convey the breakage.

I agree. I would like to get input from Josh (who is on vacation, so not tagged) or @b-butler about their takes on semver strictness and what they would do in the case of HOOMD. I would be fine with bumping to 3.0. It's also fine to just defer on this PR (or merge into a next branch) in case we want to do a review of other small changes that would be good to make for 3.0, and punt on that for a month or two before making the change.

[b-butler]

@bdice and @vyasr I would support a major version bump. For the hoomd beta's we are allowing breakages in-between beta versions, but the are not considered official releases of 3.0. I think the more people who follow semantic versioning the better as it leads to less headaches for downstream developers/scripters. I also don't think that a major version bump need be a big ordeal. Many packages have very large major versions, and I think that is fine as long as the changes are in the project's interest. The only caveat is when considering maintenance burden when supporting multiple major versions depending on the level of changes.

[vyasr]

Cool. I think a major bump would be the right way to go. I was also going to propose holding off on merging this into master until we have an idea of what other breaking changes might be useful. Offhand I recall a few other awkward locality APIs and maybe the open RDF normalization work introducing something (I'll need to look back at that eventually).

[b-butler]

@tommy-waltmann what do you think of this PR? Should we just close it or is this still in the future interest of the project in your opinion?

[bdice]

@b-butler @tommy-waltmann If you accept the API breakage, I’d like to see this feature go in when we are ready for 3.0 development. This is needed for correctness in some Voronoi cases and simplifies some analysis method code at the expense of the neighbor list requiring more information. If you would like, I can resolve conflicts and/or pull out some of the nonbreaking improvements from this PR (into separate PRs to 2.x) so that the diff is smaller. Let me know what you think.

[tommy-waltmann]

@b-butler If you have a use case (please share) motivating the use of this addition, then I think its worth continuing to pursue.

One option as far as implementation goes that is worth discussion is storing the distance as well as the vector in each NeighborBond to prevent performance regressions in any modules. The tradeoff there is memory usage and data consistency between the distances and vectors vs performance.

If we decide to continue to pursue this change, I think we will have enough momentum to start concretely planning a v3 release, making a project board for it, etc.

@bdice yes, I would agree that the base branch for this PR should be changed. I would like to see all the changes for this not be released until v3, just for better organization.

[vyasr]

I definitely think we want to move forward with this PR, and that it's just stalled waiting for a sufficient push to make v3 happen. I think the other major blocker for v3 is getting someone to address #635, which is nontrivial. If devs are willing to make a push I think that task could be completed and we'd be largely on track for a v3 release.

[tommy-waltmann]

I won't personally have much time to work on this, or other related issues for v3 until mid-August. I'm currently getting ready for multiple conferences and working on writing multiple papers. I have, however, made a next branch to accumulate changes that will be on the v3 release. Please rebase this PR so the target branch is next

[tommy-waltmann]

Regarding the only failing test, the difference between the values is ~2e-5, when the tolerance is set at 1e-5. Is the reason this is failing now just due to numerical precision, or there something deeper going on?

[tommy-waltmann]

Regarding the only failing test, the difference between the values is ~2e-5, when the tolerance is set at 1e-5. Is the reason this is failing now just due to numerical precision, or there something deeper going on?

For whatever reason this test is failing now, it appears to stem from how the neighbor distances are computed in the Voronoi module.

[tommy-waltmann]

Just made some small changes to address my own comments on this PR and push this along a bit faster.

[vyasr]

I think the next step for moving this forward is to make a plan for v3.

[tommy-waltmann]

The plan is to fix the issues on the v3 milestone, targeting the next branch and make a release when they are completed. Is there anything else you would like to see in a plan for v3?

[vyasr]

No, I just mean that I think this PR has been close to ready for a long time and the reason it stalled is the fact that it was going into v3. I don't think anything has been merged into next yet, and as soon as we start we'll have to deal with keeping it in sync with master etc so it's probably best to have a plan for all of those changes before starting the merges.

[tommy-waltmann]

What would you like to see in "a plan for all of those changes" other than what we already have? As far as merges go. I would expect that after every 2.x release, we will merge master into next until it is time to release v3.0.0

[vyasr]

I'm mostly responding to this statement:

push this along a bit faster.

Faster than what? It's not clear exactly what we're waiting on here. Is this PR ready to merge into next? If so, do devs have the bandwidth to address the remaining issues for v3 sometime soon? Maintaining a long-running next branch could also get painful when it comes to resolving conflicts depending on the scope of proposed v3 changes.

[tommy-waltmann]

The scope of the v3 changes aren't going to be massive, its not going to be like hoomd v3. I would expect that we find one or two more things that aren't on the v3 milestone currently as we address what's there, but I don't expect the number of things that need to be addressed to snowball.

Timewise, I am going to have more time in the fall than I had last summer to work towards a v3 release. The current rate of freud development on the master branch is not fast, so I wouldn't expect a lot of v2.x releases between now and when we release v3. We shouldn't have too much issue with maintaining a next branch before the v3 release.

As far as this PR goes, its just waiting for approving reviews from those who have been requested, and there is one test that is failing, likely just because of the need for a tolerance change on a floating point comparison.

[vyasr]

OK that sounds fine. In that case I'll try to review this again sometime soon.

[tommy-waltmann]

Does anyone else have any more comments or concerns on this PR?