Feature/water_models

[ape33]

This PR solves issue #3 and issue #4

[Copilot]

Pull request overview

This PR introduces dedicated water-model handling to address Issue #3 (water intermolecular interactions) and Issue #4 (water intramolecular interactions), primarily targeting SPC/Fw variants, and updates parsing/tests and a few supporting components to integrate the new model selection and behavior.

Changes:

• Add an InterWater module (strategy-based) and wire it into the build.
• Update input parsing/tests to consolidate MM-related parsing and add support for a Turbomole template keyword.
• Adjust virial APIs for const-correctness and add/extend tests (moldescriptor optional water type, hybrid zone edge case).

Reviewed changes

Copilot reviewed 106 out of 106 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
tests/src/input/testMoldescriptorReader.cpp	Adds coverage for optional waterType default state before reading the moldescriptor.
tests/src/input/inputFileParsing/testNonCoulombParser.cpp	Removes standalone noncoulomb parser test (moved into MM parser tests).
tests/src/input/inputFileParsing/testMMParser.cpp	Consolidates MM parsing tests (force-field + noncoulomb) under MMInputParser.
tests/src/input/inputFileParsing/testFilesParser.cpp	Adds test coverage for turbomole_file parsing and file existence behavior.
tests/src/input/inputFileParsing/CMakeLists.txt	Updates test sources to use testMMParser.cpp instead of older split tests.
tests/src/hybridConfigurator/testHybridConfigurator.cpp	Adds an edge-case test for zero core radius hybrid zone assignment.
tests/data/turbomoleReader/tm_define.template	Adds a Turbomole template fixture used by parsing/tests.
tests/data/inputFileReader/keywordList.txt	Updates keyword list to include turbomole_file and water/noncoulomb-related keywords.
src/waterModels/inter/interWater.cpp	Introduces InterWater dispatcher implementation, including cutoff initialization and strategy forwarding.
src/waterModels/inter/CMakeLists.txt	Adds a new interWater library target and its link/include configuration.
src/virial/virial.cpp	Makes virial calculation methods const (object constness) and keeps shift-force handling.
src/virial/molecularVirial.cpp	Adds a const, side-effect-free intramolecular virial correction tensor method.
src/simulationBox/simulationBox_standardMethods.cpp	Switches water/ammonia type getters to std::optional<size_t> and updates includes/usings.

Comments suppressed due to low confidence (1)

tests/src/input/inputFileParsing/testMMParser.cpp:78

• This test comment lists noncoulomb options as "none", "lj" and "buck", but the parser/test now supports "guff" and "morse" (and does not support "none"). Keeping the comment accurate helps prevent confusion when maintaining the parser/tests.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[ape33]

@97gamjak
I forgot to attach the overview of some benchmark calculations I have carried out.
As the comment on the bottom of the image states: only compare values within each column.
Notably: In all three columns, switching from brute force to cell list OR turning a water model on gives a speed up. However, using cell list and a water model at the same time is always slower than using no water model and just the cell list approach. So something about the implementation needs to be changed.

[galjos]

@ape33 I pushed a performance commit (7947a313) for the water model.

What it does: the inter-water cell-list kernel classifies every atom as O/H with atom->getName() == "O" per atom per pair, and the kernels read getPosition() per atom per pair — both returned by value and defined out-of-line, so each call copied across the library boundary (no cross-lib inlining with LTO off). CellList::getCells() and Cell::getNeighbourCells() also returned their vectors by value, deep-copying the whole cell list on every access. The commit returns these by const& and inlines them.

Effect: results unchanged (benchmark energy byte-identical, full test suite green); the inter-water cell-list kernel drops ~22% of its instruction count, and it helps every per-atom-pair loop in the engine. I also added benchmarks/perf/interWater.cpp since the water path had no perf-gate coverage.

One gotcha worth flagging for the water model: I tried a bigger win — computing one minimum-image shift per water molecule pair instead of per atom pair (calcShiftVector is the single largest remaining kernel cost) — and reverted it. It is not safe under the current design: PBC is applied per atom and only rcut <= L/2 is enforced (no molecular-extent margin), so for tight-but-valid boxes (e.g. L=20, rcut=9) a single per-molecule shift silently drops genuine in-cutoff O/H pairs, changing energies and forces. It only becomes safe if the cutoff is bounded by L/2 - 2*(max molecular extent) or molecules are wrapped as units. Suggest we extend the new interWater benchmark with a tight-box config so this class of regression is caught automatically.

Next candidates if useful: inline the remaining out-of-line hot getters (Cell::getAtoms/getMolecule, Molecule::isActive/getMoltype, Atom::addForce) for another ~6%, and devirtualizing the per-pair Coulomb/non-Coulomb calculate calls.

[ape33]

@galjos
nice catches, this gives indeed a massive speed-up for the cell list approach.
I just removed the AI comments and cleared the changes of the CHANGELOG.md (this file will be updated when the qmmm-dev branch gets merged into the dev branch)
However, these changes have nothing to do with the discrepancy described in comment
#223 (comment)
There is something about the implementation of the inter water interactions that is inefficient...

[ape33]

@galjos
Interesting, this last commit of yours tackles the exact issue I have described in #223 (comment)
Now the simulation times behave as expected and turning on a water model and cell list gives the most speed-up, thanks!