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)
• New feature (non-breaking change which adds or improves functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation improvement (updates to user guides, docstrings, or developer docs)

Checklist:

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

[codecov]

Codecov Report

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

❗ 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%)	⬇️
freud/util.pyx	32.22% <0.00%> (-3.87%)	⬇️
freud/box.pyx	60.89% <0.00%> (-2.14%)	⬇️
freud/order.pyx	43.72% <0.00%> (-1.33%)	⬇️
freud/diffraction.pyx	71.53% <0.00%> (-0.11%)	⬇️
freud/plot.py	83.48% <0.00%> (+0.97%)	⬆️

---

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.

[b-butler]

Overall looks good to me. I left a few comments.

[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?

[tommy-waltmann]

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

[bdice]

@tommy-waltmann A couple minor comments. Otherwise LGTM! (I can’t approve since I opened the PR.)

[b-butler]

LGTM