small modification to fix bond/react to allow equal style variables as probability input

[wverestek]

Summary

modification to fix bond/react to allow equal style variables as probability input

Related Issues

n/a

Author(s)

Wolfgang Verestek, University of Stuttgart

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

modified:   doc/src/fix_bond_react.rst
new file:   examples/USER/reaction/tiny_nylon/in.large_nylon_melt_variable_probability
new file:   examples/USER/reaction/tiny_nylon/log.22Apr20.tiny_nylon.unstabilized.g++
modified:   src/USER-REACTION/fix_bond_react.cpp
modified:   src/USER-REACTION/fix_bond_react.h

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)

[wverestek]

@jrgissing i realized that the data file is missing in the "nylon,6-6_melt" example

[jrgissing]

@jrgissing i realized that the data file is missing in the "nylon,6-6_melt" example

thanks for noticing this! (strangely, it's not the first time this data file has disappeared...)

[jrgissing]

thanks for this PR, this is a useful capability.

I will probably push some small formatting changes to your branch. also, looks like you overwrote one of the existing 'log' files in the examples, instead of making a new one

[wverestek]

thanks for this PR, this is a useful capability.

I will probably push some small formatting changes to your branch. also, looks like you overwrote one of the existing 'log' files in the examples, instead of making a new one

feel free to do so. I'm new to git and Github, but I'll do my best to proceed.

regarding the log file. As already said, new to git (except for just cloning, pulling and compiling progs). So, sorry. My intention was to provide a new example script in the tiny nylon folder to show the usage. I also put a new log file called log.22Apr20.xxx, at least that was the intention. Sorry for any inconvenience. Anything I should do now?

[jrgissing]

actually, it is just the name of your new log file that confused me. it should be something more like 'log.22Apr20.tiny_nylon.stabilized_variable_probability.g++.1' to be in line with other examples, where the number at the end is the number of processors

[wverestek]

actually, it is just the name of your new log file that confused me. it should be something more like 'log.22Apr20.tiny_nylon.stabilized_variable_probability.g++.1' to be in line with other examples, where the number at the end is the number of processors

I wasn't aware of the number at the end. Sorry, should be updated by now and follow the other examples.

[jrgissing]

extended this feature to include the Nevery, Rmin, and Rmax keywords, and updated docs

[jrgissing]

line 814 seems to be code that hasn't changed recently, are you sure that's what line you mean?

i think i did utilize 'implicit conversions' a couple times

[wverestek]

line 814 seems to be code that hasn't changed recently, are you sure that's what line you mean?

i think i did utilize 'implicit conversions' a couple times

By now I'm back home and did some small tests. Sorry, maybe i was not clear here. I thought that the variable might become "non int" during runtime. Which seems to be not an issue checking the following two examples.
"variable nevery equal step*0.5" throws an error "'Nevery must be a positive integer (src/USER-REACTION/fix_bond_react.cpp:803)"
"variable nevery equal step+(step>1)*0.5" on the other hand does not. So maybe we should mention this in the documentation that the value of the variable will be casted to int.

One other thing i found: rmin and rmax get their cutsq variables initilized. fraction is the same. nevery gets initilized for the numeric case but not for the variable case. So lines 245ff. should be

      nevery[rxn] = input->variable->compute_equal(var_id[NEVERY][rxn]);
      var_flag[NEVERY][rxn] = 1;
      delete [] str;
    } else {
      nevery[rxn] = force->inumeric(FLERR,arg[iarg]);
    }
    if (nevery[rxn] <= 0) error->all(FLERR,"Illegal fix bond/react command: "
                                       "'Nevery' must be a positive integer");
    iarg++;

in my opinion. initialized nevery in the variable case and shifted the <0 test outside.

I think the test for values < 0.0 should be also shifted one level up for rmin and rmax (around lines 270 and 290)

[jrgissing]

thanks for looking into this. would it make more sense to round the 'Nevery' variable upward, to allow more wiggle room when defining the variable?

regarding your second point, I actually purposely kept the 'sanity checks' out when first reading in the 'Nevery','Rmin','Rmax' variables, in case they were only initialized before bond/react, then modified afterward (as in the example i added to the docs). in the case of Nevery, the sanity check is instead included when Nevery is actually evaluated for use in the code

[akohlmey]

By now I'm back home and did some small tests. Sorry, maybe i was not clear here. I thought that the variable might become "non int" during runtime. Which seems to be not an issue checking the following two examples.
"variable nevery equal step*0.5" throws an error "'Nevery must be a positive integer (src/USER-REACTION/fix_bond_react.cpp:803)"
"variable nevery equal step+(step>1)*0.5" on the other hand does not. So maybe we should mention this in the documentation that the value of the variable will be casted to int.

since equal style variables are evaluated using double precision floating point math, you need to account for the fact, that certain fractions cannot be represented exactly and thus truncating to an int may give unexpected results and thus doing upward rounding by adding 0.5 might provide a more consistent behavior.

[wverestek]

thanks for looking into this. would it make more sense to round the 'Nevery' variable upward, to allow more wiggle room when defining the variable?

regarding your second point, I actually purposely kept the 'sanity checks' out when first reading in the 'Nevery','Rmin','Rmax' variables, in case they were only initialized before bond/react, then modified afterward (as in the example i added to the docs). in the case of Nevery, the sanity check is instead included when Nevery is actually evaluated for use in the code

I wanted to say that I think it would not make a big difference, it just should be documented. But I guess Axel made a point here. could we just round the value? rint( )? http://www.cplusplus.com/reference/cmath/rint/

[akohlmey]

@wverestek one recommendation for future contributions. Please do not use the "master" branch for making changes. Always create a feature branch. The master branch is best left untouched and updated from the master branch of the upstream repository, e.g. through git pull --ff-only https://github.com/lammps/lammps.git master. This will only work, if there are no commits on master. Otherwise, your master branch will incrementally diverge and every update will create needless additional commits and those will mess up any future pull requests.

[wverestek]

@wverestek one recommendation for future contributions. Please do not use the "master" branch for making changes. Always create a feature branch. The master branch is best left untouched and updated from the master branch of the upstream repository, e.g. through git pull --ff-only https://github.com/lammps/lammps.git master. This will only work, if there are no commits on master. Otherwise, your master branch will incrementally diverge and every update will create needless additional commits and those will mess up any future pull requests.

@akohlmey Thanks for the hint. @jrgissing pointed that out already. Next time I'll do that