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

Add keyword hybridpair for compute_pressure #1511

Merged
merged 8 commits into from Jul 30, 2019

Conversation

@jdevemy
Copy link
Collaborator

commented Jun 14, 2019

Add the keyword hybridpair in compute_pressure

Summary

Add the possibility to use compute_pressure only on a pair type when using hybrid or hybrid/overlay with multiple pair types

Author(s)

Alain Dequidt
Julien Devemy

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

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
  • A package specific README file has been included or updated
  • One or more example input decks are included

Further Information, Files, and Links

@mkanski

This comment has been minimized.

Copy link
Collaborator

commented Jun 14, 2019

This looks interesting, but some things are missing:

  1. You should update the docs for compute pressure.
  2. There should be a check if hybridpairflag-1 isn't larger than number of styles in pair_style hybrid. This check is needed, if there are at least two keywords used.
  3. Specifying hybridpairflag lower than zero should lead to an error.
  4. The compute should check if pair_style hybrid is actually used and not just silently ignore the keyword.

It would be nice to have an option to compute pressure from more than one substyle at once. The support for hybrid/overlay would be also appreciated.

@akohlmey

This comment has been minimized.

Copy link
Member

commented Jun 14, 2019

I would recommend against indexing the substyles, but rather use the force->pair_match() method, as it is used in other places in LAMMPS, e.g. in compute pair or fix adapt (which use similar, but slightly different command line conventions)

@akohlmey akohlmey added the needs_work label Jun 18, 2019

@jdevemy

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 21, 2019

Thanks for the comments. This request need some modifications, I agree.

But we discovered another problem: with the current pull request code, the compute_pressure hybridpair always outputs 0.
It's because the virial is computed at the pair_hybrid level and not at the subpair level.

We manage to correct this by manually adding the flag no_virial_fdotr_compute = 1 in pair_hybrid.cpp but this is not a clean way to handle this issue.

In your opinion what would be the cleanest way to deal with this issue ?

@akohlmey

This comment has been minimized.

Copy link
Member

commented Jun 21, 2019

Thanks for the comments. This request need some modifications, I agree.

But we discovered another problem: with the current pull request code, the compute_pressure hybridpair always outputs 0.
It's because the virial is computed at the pair_hybrid level and not at the subpair level.

We manage to correct this by manually adding the flag no_virial_fdotr_compute = 1 in pair_hybrid.cpp but this is not a clean way to handle this issue.

In your opinion what would be the cleanest way to deal with this issue ?

you would not hardcode this into pair style hybrid, but into compute pressure. you can check in its init() function if pair != NULL and then set pair->no_virial_fdor_compute = 1. I've always been thinking, that it might be useful to have this kind of feature as a pair_modify option (for testing/debugging). The trick is, that you may set this flag to 1, but never to 0, as some pair styles won't work with fdotr.

@jdevemy

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 25, 2019

I've updated the doc and modified the compute_pressure to use force->pair_match() and to set the flag pair->no_virial_fdor_compute = 1.

Tell-me if it's OK for you.

Regards !

@akohlmey

This comment has been minimized.

Copy link
Member

commented Jun 25, 2019

I've updated the doc and modified the compute_pressure to use force->pair_match() and to set the flag pair->no_virial_fdor_compute = 1.

Tell-me if it's OK for you.

Regards !

you must have missed something. all integration tests are failing to compile your PR branch now.

@jdevemy

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 25, 2019

you must have missed something. all integration tests are failing to compile your PR branch now.

Oups... I've forgot to add the modified compute_pressure.h. Corrected !

@akohlmey akohlmey added enhancement and removed needs_work labels Jun 25, 2019

@akohlmey akohlmey added this to the Stable Release Summer 2019 milestone Jun 25, 2019

@akohlmey akohlmey self-assigned this Jun 25, 2019

@akohlmey akohlmey requested review from sjplimp, stanmoore1 and athomps Jun 25, 2019

@stanmoore1
Copy link
Contributor

left a comment

Looks OK to me. Would be nice to add this to the other styles as well, like compute stress/atom, compute/pe, compute pe/atom, etc., but can be done in the future.

@sjplimp
Copy link
Contributor

left a comment

see 3 in-line comments

doc/src/compute_pressure.txt Outdated Show resolved Hide resolved
src/pair_hybrid.h Outdated Show resolved Hide resolved
@akohlmey

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

jdevemy added some commits Jul 2, 2019

@jdevemy

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 2, 2019

I've pushed the 2 changes requested from @sjplimp.

doc/src/compute_pressure.txt Outdated Show resolved Hide resolved
doc/src/compute_pressure.txt Outdated Show resolved Hide resolved
@sjplimp

This comment has been minimized.

Copy link
Contributor

commented Jul 2, 2019

re: having pair/hybrid set its own no_virial_compute flag correctly at each run by detecting this compute,
that's the best way to do this. But it's fine to merge this as-is (with the couple of flagged doc page changes). I can add the logic
later - it's only ~10 lines of code.

@akohlmey

This comment has been minimized.

Copy link
Member

commented Jul 29, 2019

@sjplimp this has been sitting for a while now waiting for you to approve the changes you requested.

@akohlmey akohlmey merged commit edf64ed into lammps:master Jul 30, 2019

6 checks passed

lammps/pull-requests/build-docs-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.