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

Expose neighbor lists via library interface - Second iteration #1674

Merged
merged 13 commits into from Dec 4, 2019

Conversation

@rbberger
Copy link
Member

rbberger commented Sep 12, 2019

Summary

Exposes neighbor lists via the C library interface and in the Python library.

Example (Python):

L = lammps()
L.file('in.melt')

nlocal = L.extract_global("nlocal", 0)
nghost = L.extract_global("nghost", 0)
x = L.numpy.extract_atom_darray("x", nlocal+nghost, dim=3)
v = L.numpy.extract_atom_darray("v", nlocal+nghost, dim=3)
f = L.numpy.extract_atom_darray("f", nlocal+nghost, dim=3)

# return neighbor list object, which encapsulates access via library interface

mylist = L.find_pair_neighlist("lj/cut")

# return length of list
len(mylist)
# or
mylist.size

# get neighbors of list element 0
iatom, numneigh, neighs = mylist[0]
# ^        ^       ^
# |        |       | 
# |        |       + ------- neighbors of iatom (numpy arrray of integer indices)
# |        +-------------- number of neighbors of iatom
# +--------------------- index of atom that is element 0 in neighbor list

# equivalent:
iatom, numneigh, neighs = mylist.get(0)

# neighbor list can also be used as iterator
for iatom, numneigh, neighs in mylist:
      print(x[iatom], v[iatom], f[iatom], " : ",  numneigh, "Neighbors")
      for jatom in neighs:
        if jatom < nlocal:
            print("    * [LOCAL]", jatom, x[jatom], v[jatom], f[jatom])
        else:
            print("    * [GHOST]", jatom, x[jatom], v[jatom], f[jatom])

Related Issues

#1287

Author(s)

@rbberger (Temple U)

Licensing

By submitting this pull request, I agree, that my contribution will be included in LAMMPS and redistributed under either the GNU General Public License version 2 (GPL v2) or the GNU Lesser General Public License version 2.1 (LGPL v2.1).

Backward Compatibility
yes

Implementation Notes

Adds the following library functions to the library interface:

// search for neighbor list for pair style, returns index
int lammps_find_pair_neighlist(void* ptr, char * style, int nsub, int request);

// search for neighbor list for fix, returns index
int lammps_find_fix_neighlist(void* ptr, char * id, int request);

// search for neighbor list for compute, returns index
int lammps_find_compute_neighlist(void* ptr, char * id, int request);

// get total size of neighbor list of given index
int lammps_neighlist_num_elements(void* ptr, int idx);

// return atom id, numneigh and neighbors of neighbor list element
void lammps_neighlist_element_neighbors(void * ptr, int idx, int element, int * iatom, int * numneigh, int ** neighbors);

The Python library interface uses these functions via CTypes. For ease of use they have been wrapped in a utility class called NeighList, which acts similar to a Python list. While the low level functions can be used, the lammps object provides the following utility functions which return a NeighList object. No arrays are copied, it uses numpy arrays that directly access the C memory blocks.

  def get_neighlist(self, idx):
        ....

  def find_pair_neighlist(self, style, nsub=0, request=0):
        ....

  def find_fix_neighlist(self, fixid, request=0):
        ...

  def find_compute_neighlist(self, computeid, request):
        ...

To enable the find_XYZ_neighlist functions on both the C and Python side, changes were necessary in the Neighbor List C++ code. This PR changes NeighList to include requestor, which is a pointer to the instance which asked for a neighbor list, and requestor_type, which is an enum to specify if it was a Pair, Fix or Compute. This was necessary because this information was only part of NeighRequest, which is thrown away after the list is created. I don't see a way of determining which neighbor list belongs to who without this change.

Post Submission Checklist

Please check the fields below as they are completed after the pull request has been submitted. Delete lines that don't apply

  • The feature or features in this pull request is complete
  • Licensing information is complete
  • Corresponding author information is complete
  • The source code follows the LAMMPS formatting guidelines
  • Suitable new documentation files and/or updates to the existing docs are included
  • The added/updated documentation is integrated and tested with the documentation build system
  • The feature has been verified to work with the conventional build system
  • The feature has been verified to work with the CMake based build system
  • A package specific README file has been included or updated
  • One or more example input decks are included

Further Information, Files, and Links

Put any additional information here, attach relevant text or image files, and URLs to external sites (e.g. DOIs or webpages)

Copy link
Contributor

sjplimp left a comment

Looking good - just document more for user
in library.cpp, and maybe also in lammps.py
Also need documentation added for these new
lib methods to Howto_library (8.2.5) and
Python_library (12.7)

@@ -1727,3 +1730,133 @@ int lammps_get_last_error_message(void *ptr, char * buffer, int buffer_size) {
}

#endif

/* ----------------------------------------------------------------------
Find neighbor list index for pair style

This comment has been minimized.

Copy link
@sjplimp

sjplimp Sep 13, 2019

Contributor

Document what nsub and request are?

/* ----------------------------------------------------------------------
Find neighbor list index for compute with given fix ID
The request ID identifies which request it is in case of there are
multiple neighbor lists for this fix

This comment has been minimized.

Copy link
@sjplimp

sjplimp Sep 13, 2019

Contributor

Doc that request ID is 1,2, ... Probably best not to call it an ID (string)

}

/* ----------------------------------------------------------------------
Find neighbor list index for compute with given compute ID

This comment has been minimized.

Copy link
@sjplimp

sjplimp Sep 13, 2019

Contributor

Ditto.

}

/* ----------------------------------------------------------------------
Return atom index, number of neighbors and neighbor array for neighbor

This comment has been minimized.

Copy link
@sjplimp

sjplimp Sep 13, 2019

Contributor

This needs more careful doc - which are the inputs, which are the outputs?
What is element?
Is iatom a local index or atom ID?
Say that numneigh and neighbors are length returned by lammps_neighlist_num_elements.
Not clear why there are 2 separate methods for length and these returns.
Why not have this method just return the length as the function return?
Document that the length of these are not necessarily the Nlocal for that proc.

This comment has been minimized.

Copy link
@rbberger

rbberger Nov 13, 2019

Author Member

@sjplimp lammps_neighlist_num_elements returns inum, which is the number of elements in the neighbor list. That is not the number of neighbors of a single atom (aka numneigh).

@akohlmey

This comment has been minimized.

Copy link
Member

akohlmey commented Sep 25, 2019

assigning to @sjplimp for further review and testing. please assign it back to me when satisfied.

@akohlmey

This comment has been minimized.

Copy link
Member

akohlmey commented Oct 4, 2019

@rbberger this is just a friendly reminder, that this PR is waiting for updates from you as requested by @sjplimp

@rbberger

This comment has been minimized.

Copy link
Member Author

rbberger commented Nov 13, 2019

@sjplimp please have another look. Take a closer look at the generated Python_library.html and let me know what you think.

@sjplimp

This comment has been minimized.

Copy link
Contributor

sjplimp commented Nov 15, 2019

@rbberger I like the added examples/python script. I like the (added?) Python library doc page.
However I'm not seeing any new entries for the neighbor methods on that page or the Howto_library.rst page? Do you want me to add them? I can also add a few more comments in library.cpp where I think users may look first.

@sjplimp

This comment has been minimized.

Copy link
Contributor

sjplimp commented Nov 15, 2019

@rbberger Ah, now I did a make html on the doc pages and see the Neigh list content added at the end of Python_library.html. I didn't see how that was done in the *.rst file. So that content is coming from library.cpp itself? Is the plan to do that for all the methods, both in Python_library and in Howto_library? Are you wanting to wait on this PR until that is done?

@rbberger

This comment has been minimized.

Copy link
Member Author

rbberger commented Nov 15, 2019

@sjplimp so what I did is getting source documentation from Python code (lammps.py) into the RST files. This is done through what is called autodoc. To achieve something similar with comments in C++ code, I would first have to generate an XML file using Doxygen. That would add another build dependency to the documentation build.

If you approve, I could then add markup in Howto_library.rst using something called Breathe. This will look similar to what you now see with Python code.

The alternative is to write it manually in Howto_library.rst, which is more prone to being outdated, since you're editing the docs both in the comments in library.cpp and Howto_library.rst.

@rbberger

This comment has been minimized.

Copy link
Member Author

rbberger commented Nov 15, 2019

@sjplimp the Doxygen + Breathe + Sphinx integration can now be seen in Howto_library.html. Please let me know what you think. I've limited it to library.h and library.cpp.

@sjplimp

This comment has been minimized.

Copy link
Contributor

sjplimp commented Nov 16, 2019

@rbberger What I don't understand about this syntax for either Python or cpp files is how much flexibility there is to adding comments or explanations. In the current Howto_library.rst I think the section that explains the lib methods in paragraphs is useful in addition to the prototypes. Similiarly there are text explanations in library.cpp beyond just one line per argument. Maybe if you could send me the 2 actual HTML pages and corresponding Python and library.cpp files, I would understand better.

@rbberger

This comment has been minimized.

Copy link
Member Author

rbberger commented Nov 27, 2019

Cherry-picking 5d7ce83 and 3353bff into PR #1787 so they don't get held up longer.

@sjplimp

This comment has been minimized.

Copy link
Contributor

sjplimp commented Nov 27, 2019

@rberger I'm not clear if you are waiting on me for something on this one. I am fine with the code additions. Just a good way to document both the lib and Python interface is what I am still unclear on.
In my view it doesn't have to be fully automated. I'm more interested in clarity for users, so not
all the doc info necessarily needs to be in the code.

@rbberger

This comment has been minimized.

Copy link
Member Author

rbberger commented Dec 2, 2019

@sjplimp If you don't want Doxygen and Breathe to document the library interface, say so and I'll remove the commit containing the changes.

@rbberger rbberger force-pushed the rbberger:library_interface_update branch from 5fefe63 to 2013b7e Dec 3, 2019
@rbberger

This comment has been minimized.

Copy link
Member Author

rbberger commented Dec 3, 2019

removing the Breathe/Doxygen changes for now. They will become their own PR.

@rbberger

This comment has been minimized.

Copy link
Member Author

rbberger commented Dec 3, 2019

@sjplimp please approve the PR so I can merge.

@sjplimp
sjplimp approved these changes Dec 3, 2019
@rbberger rbberger merged commit b1668f2 into lammps:master Dec 4, 2019
6 checks passed
6 checks passed
lammps/pull-requests/cmake/cmake-kokkos-omp-pr head run ended
Details
lammps/pull-requests/cmake/cmake-serial-pr head run ended
Details
lammps/pull-requests/kokkos-omp-pr head run ended
Details
lammps/pull-requests/openmpi-pr head run ended
Details
lammps/pull-requests/serial-pr head run ended
Details
lammps/pull-requests/shlib-pr head run ended
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.