Repository navigation
Fix PBAM/PBSAM regression tests and test them in CI - #323
Merged
Merged
Conversation
Restore the salt concentrations that were commented out in 2021, replace the obsolete pbsam-auto keyword with pbsam, and update the PBSAM reference energies for the solvent-dielectric scaling that pb_solvers 5671943 (2017) applied to energies to match forces. Add a PR-triggered Linux job that builds with PBAM/PBSAM enabled and runs their regression tests. Refs #321 Co-authored-by: Copilot <[email protected]> Copilot-Session: 3eaf20ea-3ac0-4701-958f-28527bf641fc
NOsh_parsePBAM and NOsh_parsePBSAM read writefmt[numwrite - 1] and writestem[numwrite - 1] even when the input had no write statement, indexing element -1. Only check the write format when a write exists. Refs #321 Co-authored-by: Copilot <[email protected]> Copilot-Session: 3eaf20ea-3ac0-4701-958f-28527bf641fc
Member
Author
|
The new PBAM/PBSAM job caught a real bug: PBSAM returned 1.25E+306 on Ubuntu while macOS gave the expected value. |
pb_solvers cef00d7 fixes a heap-buffer-overflow in BaseSystem that made PBSAM results platform dependent (Electrostatics/pb_solvers#14). Refs #321 Co-authored-by: Copilot <[email protected]> Copilot-Session: 3eaf20ea-3ac0-4701-958f-28527bf641fc
pb_solvers 8d254e5 makes PBSAM's matrix multiply work with OpenBLAS (Electrostatics/pb_solvers#15), which fixes the Linux PBSAM results. Refs #321 Co-authored-by: Copilot <[email protected]> Copilot-Session: 3eaf20ea-3ac0-4701-958f-28527bf641fc
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the PBAM and PBSAM regression tests and adds a PR-triggered CI job that builds and runs them.
Addresses the test and CI items in #321; it does not decide whether release binaries include PBAM/PBSAM.
Changes
examples/pbam/toy_energyforce.in: restoresalt 0.01, which commit fb388ee (2021) commented out.examples/pbsam-gly/gly_energyforce.in: restoresalt 0.05and replace the obsoletepbsam-autokeyword withpbsam.examples/pbsam-gly/gly_dynamics.inandgly_electrostatic.in: replacepbsam-autowithpbsamso they parse.tests/test_cases.cfgandexamples/pbsam-gly/README.md: update the PBSAM reference energies to 4.941286844454E-03 and 4.968474993771E-03..github/workflows/build.yaml: newpb-solversjob on Ubuntu 22.04 that builds with only PBAM/PBSAM enabled and runsctest -R "^(pbam|pbsam)_test$" --no-tests=error.docs/releases.rst: release note.Why the PBSAM reference energies change
pb_solvers commit 5671943 (2017) changed
EnergyCalcSAM::calc_energyto returnE*epsS.The force calculation already multiplied by
eps_s_in 2016, so the change made energies consistent with forces.New energies are exactly 80× (
sdie) the 2016 references, and the forces are unchanged to all printed digits.A ±0.05 Å finite-difference check gives energy gradients of the same order as the reported forces; without the
epsSfactor they would be about 100× too small.Testing
bb9298f):pbam_testandpbsam_testpass.pbamgives the same result.