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 stress/mop and stress/mop/profile computes for USER-MISC #1092

Merged
merged 8 commits into from Sep 17, 2018

Conversation

@RomainVermorel
Copy link
Collaborator

RomainVermorel commented Sep 3, 2018

Purpose

This pull request provides the stress/mop and stress.mop/profile compute styles. These are LAMMPS compute styles that calculate components of the local stress tensor using the method of planes as described in the paper by Todd et al. (B. D. Todd, Denis J. Evans, and Peter J. Daivis: "Pressure tensor for inhomogeneous fluids", Phys. Rev. E 52, 1627 (1995)).

Author(s)

The persons who created these files are Laurent Joly at University of Lyon 1 and Romain Vermorel at University of Pau and Pays de l'Adour.

Laurent Joly
Institut Lumière Matière
Université Lyon 1 et CNRS
43 boulevard du 11 novembre 1918
69622 Villeurbanne Cedex
e-mail: laurent.joly@univ-lyon1.fr

Romain Vermorel
Laboratory of Complex Fluids and their Reservoirs (LFCR)
Université de Pau et des Pays de l'Adour
Avenue de l'Université
64012 Pau
e-mail: romain.vermorel@univ-pau.fr

Backward Compatibility

No changes in the pull request break backward compatibility.

Implementation Notes

We verified the computations of the local stress tensor by comparing our results to some simple configurations for which abundant data is found in the literature (bulk Lennard-Jones fluid, Lennard-Jones fluid confined in a slit pore, etc). We benchmarked the code in serial and parallel, for various boundary conditions and spatial decomposition between processors.

Post Submission Checklist

Please check the fields below as they are completed

  • [x ] The feature or features in this pull request is complete
  • [ x] Suitable new documentation files and/or updates to the existing docs are included
  • [ x] One or more example input decks are included
  • [ x] The source code follows the LAMMPS formatting guidelines

Further Information, Files, and Links

@akohlmey akohlmey self-assigned this Sep 4, 2018

@akohlmey akohlmey added this to the Stable Release Fall 2018 milestone Sep 4, 2018

@akohlmey akohlmey requested review from akohlmey and sjplimp Sep 4, 2018

@RomainVermorel RomainVermorel requested review from junghans and rbberger as code owners Sep 4, 2018

@akohlmey

This comment has been minimized.

Copy link
Member

akohlmey commented Sep 4, 2018

@RomainVermorel thanks for your contribution. I've made a bunch of cosmetic changes to follow the LAMMPS style conventions and moved files from the source folder into the examples and doc tree. I've also merged to two mostly identical doc pages into one describing both commands. Please see my review message for and issue that needs fixing or explaining and a suggestion for more clarity.

@akohlmey
Copy link
Member

akohlmey left a comment

Before merging these compute styles into LAMMPS, I'd like to suggest to rename the commands from compute mop and compute mop/profile to compute stress/mop and compute stress/mop/profile. This would also require to rename the source files, documentation files (and those referencing them), example input and so on accordingly. I think that would improve clarity.

When running in parallel (with 4 MPI ranks), the output for the profile.z file differs where the parallel output has multiple rows with zeros, where the serial output has values there. this hints at some parallel processing issue when merging, outputting the data. Or if this is correct behavior, there should be an explanation in the documentation.

@akohlmey akohlmey added the enhancement label Sep 4, 2018

@RomainVermorel

This comment has been minimized.

Copy link
Owner

RomainVermorel commented on a797a0d Sep 4, 2018

Dear Axel,

Following your suggestions, I have changed the names of the computes to 'stress/mop' and 'stress/mop/profile' and modified the documentation and example files accordingly. I have also completed the comment sections of the .h files to list and describe all possible error and warning messages.

Laurent Joly and I were not able to reproduce the difference you mention between the profile.z files obtained when running lammps in serial or parallel. Moreover, the file profile.z only gathers results from the existing compute stress/atom, and was included in the example script as a comparison with the outputs from compute mop and compute mop/profile (files mopz0.time and moppz.time). Therefore if there is indeed a problem, it seems to be independent from our new computes.

If you think it would be preferable, we can remove this part from the example file and only leave the compute stress/mop and compute stress/mop/profile calculations. In the meantime, I have removed the reference output and log files.

Moreover, we consider changing the name of this package to 'USER-STRESS' as it would leave open the possibility of adding other stress computation methods in the future. Would that be suitable ?

Best regards,
Romain

@sjplimp sjplimp self-assigned this Sep 4, 2018

@akohlmey

This comment has been minimized.

Copy link
Member

akohlmey commented Sep 5, 2018

@RomainVermorel please note, that you didn't "rename" but just added new files. I had to do a significant amount of cleaning up, removing obsoleted files and changing doc entries. I have been doing this, since this is your first contribution. In the future, please make a better effort and particularly consider following the github tutorial in the LAMMPS manual more closely. Starting with not doing development in the "master" branch, but using a feature branch (after your code is merged, it is probably best to start over cleanly and drop your LAMMPS fork and make a new one, but don't do that yet).

As for the renaming of the package, this is a question to be answered by our mastermind @sjplimp
My personal perspective is, that renaming the package to something more generic implies, that you also volunteer to maintain this package. A possible alternative would be to move your contribution to the USER-MISC package.

As for differences between single and multi-processor, i see them for all generated files. Please check out the files uploaded to the pull request on github in the mop-results.tar.gz tarball.
mop-results.tar.gz

@sjplimp
Copy link
Contributor

sjplimp left a comment

yes, I like the names compute stress/mop/... better,
more descriptive. I also think the 2 new commands
could simply be added to USER-MISC for now. No need for a new package. If we end up with more stress options at some point, we can create a new package later, i.e. USER-STRESS.

@akohlmey akohlmey assigned rbberger and unassigned akohlmey and sjplimp Sep 5, 2018

@akohlmey

This comment has been minimized.

Copy link
Member

akohlmey commented Sep 5, 2018

moved the styles of USER-MISC. @rbberger it is all yours now

@RomainVermorel

This comment has been minimized.

Copy link
Collaborator

RomainVermorel commented Sep 6, 2018

@akohlmey sorry for the inconvenience caused, I am new to GitHub... I must have overlooked the part of the tutorial on the feature branch. Next time I will pay more attention.

I suppose the differences between serial and parallel runs resulted from the initialization of the system in the example input script. We indeed used some random procedures for deleting some atoms and creating initial velocities, which led to different results depending on the number of processors.

We have prepared a new example input script and a data file to make sure the initial conditions are the same, irrespective of the number of processors. This simulation provides the same output data with single and multi-processors. Please check out these new input scripts in the attached file and let me know if it solves the issue.

mop_example_20180906.tar.gz

akohlmey added some commits Sep 7, 2018

@akohlmey

This comment has been minimized.

Copy link
Member

akohlmey commented Sep 7, 2018

@RomainVermorel thanks for the updated files, I've applied them to the pull request and resolved a conflict with changes that have happened to the master branch since. This looks ready to go in now. I've assigned it to @rbberger for final review and merge.

@akohlmey akohlmey changed the title new USER-MOP package submitted new stress/mop and stress/mop/profile computes for USER-MISC Sep 7, 2018

@sjplimp sjplimp self-requested a review Sep 14, 2018

@akohlmey akohlmey merged commit 0c287a5 into lammps:master Sep 17, 2018

5 checks passed

lammps/pull-requests/build-docs-pr head run ended
Details
lammps/pull-requests/kokkos_omp 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