Closed pfebrer closed 3 weeks ago
I really like this, however, I am a bit worried about the complexity of the different classes? Are there too many, should we coerce them into 1 with flags to denote their content? What would a user expect, for generic functionality they would have to do lots of isinstance where this could possibly be leveraged by have_symmetric_neighbors or similar, I am not saying that is the way to go, but we should at least consider it?
The first implementation was actually a single class with flags, but the logic inside each method/property was kind of a mess. The code was not easy to read. Also depending on the thing stored, the functionality changes significantly and attributes with the same name mean different things. So I think it is clearer for the user if each type of return is a different class. Also there wasn't much interplay between the different flags really, so in the end you would have flags that are something like is_class_x
which would be equivalent to the current isinstance(class_x)
.
True... It might be fine like this!
I changed it so that the iterator works with __getitem__
, to allow directly getting the neighbors of a given atom instead of requiring the user to loop. (code snippet in the description is updated)
@pfebrer what should I help with here?
I can do it myself, but probably next week.
Attention: Patch coverage is 51.25000%
with 117 lines
in your changes missing coverage. Please review.
Project coverage is 87.28%. Comparing base (
81dd25e
) to head (48d4c02
). Report is 2 commits behind head on main.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Done! Everything is there, including docs. You can reorganize the docs as you wish.
Thanks!
isort .
andblack .
[24.2.0] at top-leveldocs/
CHANGELOG.md
As discussed in #744, this PR modifies the return values of the neighbor finder so that the user has to ask explicitly for a certain quantity, making sure that they get what they want.
There is a general
Interactions
class, but the different ways in which the finder can work require slightly different manipulations, and I have created multiple classes to facilitate that the user notices what they have at hand.For finding neighbors (atom-atom interactions), we have the following classes:
FullNeighborsList
: Contains all the neighbor interactions in the system. Returned byfind_neighbors
ifatoms
isNone
.PartialNeighborsList
: Contains the neighbors for a list of atoms (neighbors can be outside that list). Returned byfind_neighbors
ifatoms
is notNone
.UniqueNeighborsList
: Contains all the neighbor interactions in the system, but only on one direction. Returned byfind_unique_pairs
. This is like the upper triangular part of the sparsity pattern/adjacency matrix.AtomNeighborsList
: Contains the neighbors for a single atom. This is returned when iterating overFullNeighborsList
orPartialNeighborsList
.For finding atoms that are close to certain points in space we have:
CloseAtoms
: Contains all the interactions between points and atoms. Returned byfind_close
.PointCloseAtoms
: Contains the atoms that are close to a certain point. Returned when iterating overCloseAtoms
.This is not finished, but before finishing the details I would like to get your opinion to see if you think it is on the right track or not.
One thing I noticed is that the
FullNeighborsList
andUniqueNeighborsList
are very similar to aSparseAtoms
object. I don't know if we should return that instead, or subclassSparseAtoms
. Although with a neighbors list it is probably more convenient to have the matrix in COO format.Here is an example of how one might get and use a
FullNeighborsList
: