Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LocalDescriptors num_neighbors has an unclear name #196

Closed
bdice opened this issue Nov 24, 2018 · 4 comments
Closed

LocalDescriptors num_neighbors has an unclear name #196

bdice opened this issue Nov 24, 2018 · 4 comments
Labels
bug Something isn't working environment
Milestone

Comments

@bdice
Copy link
Member

bdice commented Nov 24, 2018

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, @vyasr @klarh?

@bdice
Copy link
Member Author

bdice commented Nov 24, 2018

...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.

@bdice
Copy link
Member Author

bdice commented Nov 24, 2018

Original comment by Vyas Ramasubramani (Bitbucket: vramasub, GitHub: vyasr).


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.

@bdice
Copy link
Member Author

bdice commented Nov 26, 2018

Original comment by Matthew Spellings (Bitbucket: mspells, GitHub: klarh).


That quantity definitely shouldn't be called num_neighbors. It is the number of bonds, but I neglected to update the docstring in f9ea60d when I updated the result of LocalDescriptors from being a Np*Nneigh*Nsphs array to a Nbonds*Nsphs array, causing the property to be added in 367edfd. There could conceivably be a value in returning the last num_neighbors value given in LocalDescriptors.compute, but I think it certainly shouldn't be spitting out the number of bonds when it's called what it currently is.

@bdice
Copy link
Member Author

bdice commented Jan 18, 2019

API change, punting to 2.0.

@bdice bdice added minor bug Something isn't working environment and removed minor labels Jan 23, 2019
vyasr pushed a commit that referenced this issue Jan 27, 2019
Improved tbb error checking.

Approved-by: Vyas Ramasubramani <vramasub@umich.edu>
@bdice bdice removed the 2.0 label Feb 9, 2019
@bdice bdice added this to the 2.0 milestone Feb 9, 2019
@bdice bdice closed this as completed Aug 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working environment
Projects
None yet
Development

No branches or pull requests

1 participant