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

New developer page for comm functions #3146

Merged
merged 5 commits into from
Feb 23, 2022
Merged

New developer page for comm functions #3146

merged 5 commits into from
Feb 23, 2022

Conversation

sjplimp
Copy link
Contributor

@sjplimp sjplimp commented Feb 21, 2022

Summary

New doc page for Programmer Guide which describes various communication operations provided in LAMMPS. Which users writing new style classes can invoke from their new code.

Related Issue(s)

N/A

Author(s)

Steve

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

Implementation Notes

Post Submission Checklist

  • 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
  • Suitable tests have been added to the unittest tree.
  • A package specific README file has been included or updated
  • One or more example input decks are included

Further Information, Files, and Links

@sjplimp
Copy link
Contributor Author

sjplimp commented Feb 21, 2022

@akohlmey Here is draft of the new doc page we talked about. Not sure where in the Developer section you envision it going.

@akohlmey
Copy link
Member

@akohlmey Here is draft of the new doc page we talked about. Not sure where in the Developer section you envision it going.

thanks. How do you want to proceed?

  • should I do a pass editing it and then you check it out again and finalize it?
  • should I add comments with suggestions and you do the edits?

@sjplimp
Copy link
Contributor Author

sjplimp commented Feb 21, 2022

1st bullet is fine

The *Fix*, *Compute*, and *Dump* classes can invoke the same forward
and reverse communication operations using these *Comm* class methods:

* forward_comm_fix()
Copy link
Member

@akohlmey akohlmey Feb 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sjplimp Do you remember a particular reason why there is forward_comm_pair(), forward_comm_fix() and so on instead of just forward_comm()?
With the exception of forward_comm_fix_variable() those are all uniquely identifiable from the type of the first argument. So we could have them all named forward_comm() and the variable one forward_comm_variable().

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

implemented in #3148. The text here is updated to reflect that PR.

@sjplimp
Copy link
Contributor Author

sjplimp commented Feb 21, 2022

You mean just change the method names, not template them?
That would be fine I guess. What is the int dummy arg for forward_comm() ?
Maybe a Kokkos thing? Will prototyping on the 1st arg mess up Kokkos?

@akohlmey
Copy link
Member

akohlmey commented Feb 21, 2022

You mean just change the method names, not template them?

yes. I don't think they can be templated. Won't save us anything. This is more a consistency thing, i.e. avoid the "hungarian notation", i.e. have the function name encodes the type of arguments. That is needed in C when you cast to void *.
I am not 100% convince this is a good idea. Having the same function do the same procedure is good and overloading useful, but those are also virtual functions and thus overloading them makes it harder to detect incorrect use.

That would be fine I guess. What is the int dummy arg for forward_comm() ? Maybe a Kokkos thing? Will prototyping on the 1st arg mess up Kokkos?

Should be OK, but for certain such a change would have to be a separate pull request with all uses updated and tested.

@akohlmey
Copy link
Member

1st bullet is fine

@sjplimp I have added my edits and also integrated it into the manual. For now, I just left it as a separate file and made it a new toc item. Looks quite good to me. Has pretty much everything I was looking for. I only added a few more details for better clarity. Fixed some spelling and formatting things (including your request for subheadings). Please check it out.

Please note, that with the :doc:`newton off <newton>` setting this does
not happen and the neighbor lists are constructed that these pairs are
computed on both MPI processes containing one of the atoms and only the
data pertaining to the local atom is stored.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'stored' -> 'accumulated'

*forward_comm()* and *reverse_comm()* call which are used when there are
communications with buffer sizes different from the value set in the
comm_forward/comm_reverse variables. For this to work a class member
variable must be set to indicate to the pack/unpack routines which
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

which. which -> which

@athomps
Copy link
Contributor

athomps commented Feb 22, 2022

One additional thing that could be mentioned near the top of this section or elsewhere is that ghost atoms and forward/reverse communication are also used to implement periodic boundary conditions.

@akohlmey akohlmey merged commit d848e50 into develop Feb 23, 2022
@akohlmey akohlmey deleted the programmer-comm-doc branch February 23, 2022 01:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants