- changed title to LocalDescriptors num_neighbors has an unclear name
LocalDescriptors num_neighbors has an unclear name
The property num_neighbors
in LocalDescriptors
isn't a very clear description - the value actually represents the "Last number of bond spherical harmonics computed" (as described in the C++ file), which is equal to the number of bonds in the computed neighbor list.
I suggest a rename to num_sphs
or num_bonds
or similar. To me, num_neighbors
sounds more like the k in k-nearest-neighbors, which won't scale with the number of particles present.
@property
def num_neighbors(self):
return self.thisptr.getNSphs()
def getNSphs(self):
warnings.warn("The getNSphs function is deprecated in favor "
"of the num_neighbors class attribute and will be "
"removed in a future version of freud.",
FreudDeprecationWarning)
return self.num_neighbors
Thoughts, @vramasub @mspells?
Comments (5)
-
reporter -
reporter ...maybe this is fine. I read the Python docs again and it gives an explanation of the value, but I'm still not very fond of the property name. Current description:
num_neighbors (unsigned int): The number of neighbors used by the last call to compute. Bounded from above by the number of reference points multiplied by the lower of the num_neighbors arguments passed to the last compute call or the constructor.
-
IIRC I rewrote both the cpp and python docstrings for this attribute to clarify precisely this point, but I didn't actually change the name. We could change the name too if we wanted to. We'd have to be careful not to clash with the existing naming of things in the Local Descriptors documentation is all.
-
That quantity definitely shouldn't be called
num_neighbors
. It is the number of bonds, but I neglected to update the docstring in f9ea60ded when I updated the result ofLocalDescriptors
from being aNp*Nneigh*Nsphs
array to aNbonds*Nsphs
array, causing the property to be added in 367edfdcf. There could conceivably be a value in returning the lastnum_neighbors
value given inLocalDescriptors.compute
, but I think it certainly shouldn't be spitting out the number of bonds when it's called what it currently is. -
reporter - changed version to 2.0
API change, punting to 2.0.
- Log in to comment