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

Back-porting of Zombie SNAP improvements #1507

Merged
merged 21 commits into from Jun 17, 2019

Conversation

@athomps
Copy link
Contributor

commented Jun 12, 2019

Summary

Completed back-porting of Zombie SNAP improvements, particularly noteworthy is reduction in memory footprint, elimination of most multidimensional arrays, elimination of diagonal_style, elimination of Z array in force calculation, gives about 2x speed-up on typical benchmarks.

Related Issues

Author(s)

Aidan Thompson

Licensing

Backward Compatibility

SNAP param files that contain the diagonalstyle keyword will generate this error:

ERROR: Incorrect SNAP parameter file

Solution is to remove the diagonalstyle keyword. The code now supports only diagonalstyle=3. This change has been documented.

Implementation Notes

The three SNAP potentials in examples/snap work as before. Additional testing of old versus new on output from compute sna, snad, snav, and quadratic SNAP was performed, with only tiny differences < 1e-10.

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)

athomps added some commits Apr 22, 2019

Completed back-porting of Zombie SNAP improvements, particularly note…
…worthy is reduction in memory footprint, elimination of most multidimensional arrays, elimination of diagonal_style, elimination of Z array in force calculation.

@athomps athomps requested a review from stanmoore1 Jun 12, 2019

@akohlmey

This comment has been minimized.

Copy link
Member

commented Jun 13, 2019

@athomps this PR seems to break the KOKKOS version of SNAP. There also seems to be a typo in the docs ("ompute" instead of "compute", if i read the error logs correctly).

@akohlmey

This comment has been minimized.

Copy link
Member

commented Jun 13, 2019

@athomps BTW, when editing the checklist, you must delete the extra blank so it renders the markup correctly. or better yet, set the entries to enabled by clicking on the buttons after submitting the PR.

@athomps

This comment has been minimized.

Copy link
Contributor Author

commented Jun 13, 2019

I updated KOKKOS package. I did not find the ompute typo.

Show resolved Hide resolved doc/src/compute_sna_atom.txt Outdated

@akohlmey akohlmey changed the title Yarray Back-porting of Zombie SNAP improvements Jun 13, 2019

@akohlmey akohlmey assigned akohlmey and unassigned athomps and stanmoore1 Jun 13, 2019

@akohlmey akohlmey requested a review from sjplimp Jun 13, 2019

@akohlmey akohlmey removed the needs_work label Jun 13, 2019

@sjplimp
Copy link
Contributor

left a comment

good to go

@akohlmey akohlmey merged commit 93fd33a into lammps:master Jun 17, 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
@@ -233,66 +162,22 @@ void PairSNAP::compute_regular(int eflag, int vflag)
}
}

// compute Ui, Zi, and Bi for atom I
// compute Ui, Yi for atom I

snaptr->compute_ui(ninside);

This comment has been minimized.

Copy link
@stanmoore1

stanmoore1 Jun 20, 2019

Contributor

@athomps Isn't this redundant if quadratic_flag is set? In other words it should be conditional on !quadratic_flag?

@athomps

This comment has been minimized.

Copy link
Contributor Author

commented Jun 20, 2019

@stanmoore1

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2019

I mean for quadratic, compute_ui is already called previously in compute_bispectrum(), so it is getting called twice.

@athomps

This comment has been minimized.

Copy link
Contributor Author

commented Jun 20, 2019

@stanmoore1

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2019

That makes sense, thanks

@athomps athomps deleted the athomps:yarray branch Jun 20, 2019

@athomps

This comment has been minimized.

Copy link
Contributor Author

commented Jun 20, 2019

I did a test of making compute_bispectrum conditional on eflag, not quadraticflag, which gives somewhat incorrect results, but is representative. The time reduction was about 20%. Probably not worth exploiting by itself, but something to keep in mind.

@stanmoore1

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2019

@athomps lines 613-614 of sna.cpp look like no-ops. Can you confirm? See https://github.com/lammps/lammps/blob/master/src/SNAP/sna.cpp#L613

@athomps

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 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.