fix_gcmc.cpp question

Hello,

In the 30 Dec 2013 patch fix_gcmc.cpp was changed:

diff -Naur lammps-17Dec13/src/MC/fix_gcmc.cpp lammps-30Dec13/src/MC/fix_gcmc.cpp
@@ -1156,6 +1156,7 @@
   double com[3];
   gas_mass = group->mass(0);
   group->xcm(0,gas_mass,com);
+ gas_mass /= comm->nprocs;

   double **x = atom->x;
   for (int i = 0; i < nlocal; i++) {

In the 31 Dec 2013 patch the line added the previous day was then removed:

diff -Naur lammps-30Dec13/src/MC/fix_gcmc.cpp lammps-31Dec13/src/MC/fix_gcmc.cpp
@@ -1156,7 +1156,6 @@
   double com[3];
   gas_mass = group->mass(0);
   group->xcm(0,gas_mass,com);
- gas_mass /= comm->nprocs;

   double **x = atom->x;
   for (int i = 0; i < nlocal; i++) {

It seems to me that the line "gas_mass /=comm->nprocs;" is needed
since the value of gas_mass produced by group->mass(0) is off by a
factor equal to nprocs (even in the 1Feb14 stable release).

Thanks for your time,

CBL

Hello,

In the 30 Dec 2013 patch fix_gcmc.cpp was changed:

diff -Naur lammps-17Dec13/src/MC/fix_gcmc.cpp lammps-30Dec13/src/MC/fix_gcmc.cpp
@@ -1156,6 +1156,7 @@
   double com[3];
   gas_mass = group->mass(0);
   group->xcm(0,gas_mass,com);
+ gas_mass /= comm->nprocs;

   double **x = atom->x;
   for (int i = 0; i < nlocal; i++) {

In the 31 Dec 2013 patch the line added the previous day was then removed:

diff -Naur lammps-30Dec13/src/MC/fix_gcmc.cpp lammps-31Dec13/src/MC/fix_gcmc.cpp
@@ -1156,7 +1156,6 @@
   double com[3];
   gas_mass = group->mass(0);
   group->xcm(0,gas_mass,com);
- gas_mass /= comm->nprocs;

   double **x = atom->x;
   for (int i = 0; i < nlocal; i++) {

It seems to me that the line "gas_mass /=comm->nprocs;" is needed
since the value of gas_mass produced by group->mass(0) is off by a
factor equal to nprocs (even in the 1Feb14 stable release).

yes, looking at the detailed commit history in the git repository,
this one-line fix seemed have been lost when resolving problems
related to new features. this is a risk that originates in certain
development workflows that are not easy to change. we are currently
discussing plans to set up some more automatic tools to check for
these things. if you have a simple LAMMPS input script that would
demonstrate the difference, we can include it in future tests.

axel.

Axel,

Thanks for the reply. Im not sure of a way to directly print gas_mass
from a LAMMPS input script. However as discussed back in Dec,
(http://lammps.sandia.gov/threads/msg43324.html), when running in
parallel the incorrect gas_mass value can cause incorrect molecular
rotations. At that time I provided a simple input script that showed
after a successful rotation, a molecule's COM was moved outside of the
defined region. The line "gas_mass /= comm->nprocs;" fixed the issue.
So would that input script work for you as a check in an indirect way?
If so, I will tighten it up a bit and repost it here.

As an aside, it seems that even with the "gas_mass /= comm->nprocs;"
line, I still get different results in parallel vs serial when running
the gcmc_mol script I provided here
-->(http://lammps.sandia.gov/threads/msg43011.html). Which indicates
there still is a problem in the code somewhere.

Again, thanks for your time,

CBL

Axel,

Thanks for the reply. Im not sure of a way to directly print gas_mass
from a LAMMPS input script. However as discussed back in Dec,
(LAMMPS Molecular Dynamics Simulator), when running in
parallel the incorrect gas_mass value can cause incorrect molecular
rotations. At that time I provided a simple input script that showed
after a successful rotation, a molecule's COM was moved outside of the
defined region. The line "gas_mass /= comm->nprocs;" fixed the issue.
So would that input script work for you as a check in an indirect way?
If so, I will tighten it up a bit and repost it here.

yes. anything where a deviation from the reference reliably shows a
problem is a suitable example. one problem with (parallel) monte carlo
operations is that one needs to guarantee a certain sequence of
(pseudo) random numbers to identify such a deviation, particularly
between different numbers of processors. thus we might need to insert
some additional code for exactly that purpose.

As an aside, it seems that even with the "gas_mass /= comm->nprocs;"
line, I still get different results in parallel vs serial when running
the gcmc_mol script I provided here
-->(LAMMPS Molecular Dynamics Simulator). Which indicates
there still is a problem in the code somewhere.

paul crozier (cc'd) is the person that is usually tasked with the
burden of having a closer look. since we are all volunteering our time
for the maintenance of LAMMPS, sometimes things don't always move
fast. i have started to use the issue tracker here (
Log in with Atlassian account ) to keep track of known
problems and encourage everybody that notices a problem with the code
to submit a bug report here, so that it doesn't get lost. as the
LAMMPS community and the code complexity is growing, we are exploring
additional ways to improve the overall workflow. this is one attempt
at that.

axel.

I've attached a sample input script (gcmc.in) and output (log.lammps).
When this script is run on 8 procs without the line "gas_mass /=
comm->nprocs;" in fix_gcmc.cpp, it demonstrates that a successful
rotation move takes a molecule outside of the defined region. The last
column of the thermo output (Mtot_Min) is the total number of
molecules in the system minus the number of molecules in the defined
region (used in fix_gcmc). On step 5 of the thermo output (see
log.lammps) there is only a successful rotation, and the last column
indicates that there is a molecule outside the defined region. When
"gas_mass /= comm->nprocs;" is added to the source code, this rotation
error no longer occurs.

I added an option to fix_gcmc that prevents any move (translation,
rotation, insertion) from moving any part of a molecule outside of the
defined region. As discussed in the manual and previously on the
mailing list, moves can take an atom or molecule slightly outside of
the specified region. If it is of general interest to the community I
will clean up the code and post it here. That will also give me a
chance to go through the source code again and search for any bugs.

Thanks,

-CBL

gcmc.in (1.81 KB)

log.lammps (4.79 KB)

Paul “owns” the fix_gcmc.cpp files, so he can
comment on the proposed changes or previous bug …

Then we will update the master version if needed.

Steve

Yes, Charles was right that “gas_mass /= comm->nprocs;” change I made back in December was needed. I’ve re-committed the change. Looks like Steve inadvertently reverted the change I’d made. Thanks for flagging this Charles. And yes, better testing would be a good thing, so thanks for sending the candidate test case.

Paul

it will be in the next patch - thanks

Steve