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

Adding the conjugate gradient algorithm and L-BFGS to the SPIN package #1598

Merged
merged 87 commits into from Oct 5, 2019

Conversation

@alxvov
Copy link
Collaborator

alxvov commented Jul 30, 2019

Summary

  • Implementation of conjugate gradient algorithm (Fletcher-Reeves algorithm ) and limited-memory Broyden-Fletcher-Goldfarb-Shanno for spin system.
  • Adding a max_norm option to all minimisers. max_norm - maximum force/torque on atom.

Related Issues

improvement of the SPIN package and of the minimizers

Author(s)

Aleksei Ivanov, University of Iceland, alxvov@gmail.com
Julien Tranchida, Sandia National Laboratories, jtranch@sandia.gov

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

n/e

Implementation Notes
The correctness of the implementation was verified as follows. Firstly, analytical spin torques of energy were checked against numerical torques using finite difference approach. Secondly, several local minima were identified using new algorithms. These magnetic states coincide with the states obtained by dissipative spin dynamics approach. Energy of these states (including magnetic skyrmions) are in agreement with the literature.

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
alxvov and others added 30 commits Jun 27, 2019
@julient31

This comment has been minimized.

Copy link
Collaborator

julient31 commented Sep 4, 2019

Axel @akohlmey and Richard @rbberger, Aleksei and I have been correcting the documentation and modified the file names as we discussed.
Axel @akohlmey, if you don't mind, I'll push the fix nve/spin modifications we discussed in a separate PR.
Overall, I think this one is ready. Let us know if you see something you would like us to modify.

Copy link
Contributor

athomps left a comment

Here are a few suggestions:

  1. Don't include Makefile.serial in pull request
  2. Looking at min_cg.cpp, I see that the max operation is over all force components, but the documentation says "The {max} norm computes the maximum value of the 2-norms across all forces in the system" which sounds ambiguous and could mean "maximum of the 2-norms of all atoms in the system." I actually like that definition better, as it is invariant to rotation of the coordinate frame. Also, how is max calculated for the case of torque?
The choice of a norm can be modified for the min styles {cg}, {sd},
{quickmin}, {fire}, {spin}, {spin/cg} and {spin/lbfgs} using
the {norm} keyword.
The default {euclidean} norm computes the 2-norm (length) of the

This comment has been minimized.

Copy link
@sjplimp

sjplimp Sep 9, 2019

Contributor

Here is where to explain exactly what "max" means.

backtrack,quadratic,forcezero = style of linesearch to use
{line} value = {backtrack} or {quadratic} or {forcezero} or {spin_cubic} or {spin_none}
backtrack,quadratic,forcezero,spin_cubic,spin_none = style of linesearch to use
{norm} value = {euclidean} or {max}

This comment has been minimized.

Copy link
@sjplimp

sjplimp Sep 9, 2019

Contributor

I suggest:
norm value = two or max
two = Euclidean two-norm (length of 3N vector)
max = max absolute value of any 3N vector component

I think "max" is the option you are adding? As Aidan asked (I think)
is this the true max norm (across all components), or
are you taking the length of the 3-vector for each atom (2-norm)
and taking the max of those across all atoms? That should be explained below.

This comment has been minimized.

Copy link
@sjplimp

sjplimp Sep 9, 2019

Contributor

And the name of what I called the true max norm is the infinity norm, correct?
If that's what you want, best to call it that.

backtrack,quadratic,forcezero = style of linesearch to use
{line} value = {backtrack} or {quadratic} or {forcezero} or {spin_cubic} or {spin_none}
backtrack,quadratic,forcezero,spin_cubic,spin_none = style of linesearch to use
{norm} value = {euclidean} or {max}

This comment has been minimized.

Copy link
@sjplimp

sjplimp Sep 9, 2019

Contributor

And the name of what I called the true max norm is the infinity norm, correct?
If that's what you want, best to call it that.

@@ -250,7 +250,15 @@ int MinFire::iterate(int maxiter)
// sync across replicas if running multi-replica minimization

if (update->ftol > 0.0) {
fdotf = fnorm_sqr();
if (normstyle == 1) { // max force norm

This comment has been minimized.

Copy link
@sjplimp

sjplimp Sep 9, 2019

Contributor

Better to use a enum value for normstyle == 1, e.g. normstyle == INFINITY or normstyle == EUCLIDEAN

This comment has been minimized.

Copy link
@sjplimp

sjplimp Sep 9, 2019

Contributor

In case we add other options later.

julient31 added 3 commits Sep 23, 2019
- modified norm input in min_modify
- corrected doc/src/min_modify.txt
- added expression of the norms
- added a min max method in src/min.h
- added enum to min.h (for norm choice)
- completed doc min_modify
- corrected torque tol issue in spin/min
- corrected a typo in doc/src/min_modify.txt
@julient31

This comment has been minimized.

Copy link
Collaborator

julient31 commented Sep 23, 2019

@athomps , I removed the modifications in Makefile.serial. I still don't remember where this was coming from (there was a --wall).
I modified the force norms (for cg, sd, fire, and quickmin) so that they match the appropriate definition: "computes the length of the 3-vector force for each atom (2-norm), and takes the maximum value of those across all atoms". The definitions are the same for the magnetic torques.

@sjplimp , I implemented all the changes you were asking for, but one: there is already an inf norm in Lammps (the max force component across all atoms). See min.ccp, line 803.
Instead, I created a new fnorm method, fnorm_max (see line 834 of min.cpp) for the max norm Aidan was discussing. For consistency, I let the new norm style called "max".
From what I've seen, it seems to be fine to call this norm either "max" or "infinite".

julient31 added 3 commits Sep 23, 2019
- corrected src/min.h merging conflict
- added inf norm option
@julient31

This comment has been minimized.

Copy link
Collaborator

julient31 commented Sep 24, 2019

@sjplimp after a brief discussion with @athomps ( ;-) ), I actually added the infinite norm too. This was actually not a lot of work, and will probably avoid some confusion.
To summarize, the users now have three norm options:

  • "two", the old one (and also the default one),
  • "max", the max force norm across all atoms,
  • "inf", the max force component across all atoms.

All this is documented (I provided mathematical expressions for each of the three norms) in min_modify.txt.

@akohlmey akohlmey requested a review from sjplimp Oct 4, 2019
@akohlmey

This comment has been minimized.

Copy link
Member

akohlmey commented Oct 4, 2019

@julient31 @alxvov this PR has now a conflict with the master branch due to my merging of #1692. Please check it out and update accordingly. I would like to merge this PR for the next patch release.

@sjplimp
sjplimp approved these changes Oct 4, 2019
- solved conflict in doc/src/fix_nve_spin.txt
@julient31

This comment has been minimized.

Copy link
Collaborator

julient31 commented Oct 4, 2019

@akohlmey , I did the merge and solved the conflict. Thanks a lot Axel!

@akohlmey akohlmey merged commit 65edb2a into lammps:master Oct 5, 2019
8 checks passed
8 checks passed
lammps/pull-requests/build-docs-pr head run ended
Details
lammps/pull-requests/cmake/cmake-kokkos-cuda-pr head run ended
Details
lammps/pull-requests/cmake/cmake-kokkos-omp-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
@alxvov alxvov deleted the alxvov:OSO branch Oct 8, 2019
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.