Conversation
…rostatic path Upstream openmm#167 restricts PythonForce to the ML atoms via setParticles(), so the callback state no longer carries MM coordinates. PolarMACE electrostatic embedding needs them in the same forward pass (the MM field polarises the ML density) and returns MM back-reaction forces, so that path keeps the explicit index slice/scatter and does not call setParticles(). The plain ML path uses upstream's restriction unchanged; with indices=None and mmInfo=None _computeMACE is behaviourally identical to upstream. Same pattern as the EMLE embedding PR (openmm#159): a full-system PythonForce that indexes ML and MM regions itself. Verified on OpenMM 8.6.1 (isolated env) with graph_longrange@external_field: TestPolarMACEElectrostaticEmbedding + TestElectrostaticEmbeddingPME + TestMechanicalEmbedding: 104 passed, 6 skipped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ents Style alignment with openmmml/embeddings/emleembedding.py (upstream PR openmm#159), which is the reference for how an ML/MM embedding is written here. No behaviour change: the mixed System fingerprint (every NonbondedForce parameter and exception, CustomNonbondedForce exclusions and parameters, plus energy and forces from a Reference Context at fixed positions, PBC and non-PBC, against one saved PolarMACE model) is byte-identical to before, energies -84.29790229105993 and -56.26526271299811 kJ/mol. Tests: 104 passed, 6 skipped. - camelCase for identifiers we own: modelDevice, positionsFull, includedAtoms, useMMEmbedding, mlAtoms/mmAtoms/mmCharges/mmPositions/mmForces, and the private helpers _supportsMMEmbedding, _prepareExternalSources, _enablePolarMACEExternalSources. External API names are untouched and must stay snake_case: the MACE input/result dict keys ("mm_positions", "mm_charges", "mm_source_batch", "external_field", "node_attrs", "fermi_level", "mm_forces"), the compute_* and requires_grad kwargs, and the mace_off/mace_mp/mace_polar model-family strings. - The mmInfo dict no longer crosses into the callback. _prepareMMEmbedding is unpacked at the call site and the values are bound by name, as emleembedding.py does with mlIndices/mmIndices/ mmCharges, so _computeMACE's signature documents its own inputs: def _computeMACE(state, model, ptr, nodeAttrs, batch, pbc, returnEnergyType, charge, multiplicity, periodic, mlIndices=None, mmIndices=None, mmCharges=None) The electrostatic path is selected by `mmIndices is not None` exactly where it previously used `mmInfo is not None`, and still does not call setParticles(). - _shouldUseMMEmbedding both raised and returned a flag, and one call site discarded the flag, so it read as a no-op. It is now _validateMMEmbedding, which only raises; the flag is derived where it is used (`useMMEmbedding = embedding == "electrostatic"`). Its "unsupported mode" message now names what MACE implements and what it delegates. - Wrapped the four error messages this branch added that ran to 122-187 characters, and split the _computeMACE signature over two lines. Upstream's own long lines and the KNOWN_MODELS URL table are left alone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011TXm1UZZ1hYRkFHZgVFwAN
| pytest | ||
| pygit2 | ||
| mace-torch | ||
| loguru | ||
| torchani==2.2.4 | ||
| setuptools<81 | ||
| git+https://github.com/chemle/emle-engine.git@6cb9bbd14008788ae83bd9d39a27b11e0e3407f7 |
There was a problem hiding this comment.
Hopefully this won't be needed once we release the base emle-engine package on conda-forge.
| | Name | Model | | ||
| | --- | --- | | ||
| | `embeddingModelPath` | Path to a local model, only used (and required) if `emle-engine` is given as the embedding name. | | ||
| | `alphaMode` | The mode for computing atomic polarizabilities: can be `'species'` (default) or `'reference'`. See the [EMLE-Engine API documentation](https://chemle.github.io/emle-engine/api/index_models.html#emle.models.EMLE) for more details. | |
There was a problem hiding this comment.
Note that alphaMode will change in the next release. 'species' will become 'fixed', and 'reference' will become 'flexible'. See https://github.com/chemle/emle-engine/blob/devel/emle/models/_emle.py#L116-L122
There was a problem hiding this comment.
I'll update this documentation, and also change the code to not pass the keyword argument to EMLE if the user doesn't specify it; this way, the default won't be hard-coded in OpenMM-ML.
| | `switchingDistance` | The switching distance for EMLE. Must be an `openmm.unit.Quantity` with distance units. Between this distance and the cutoff distance from the ML region, the effect of MM atoms will go smoothly to zero. The default value is 0.6 nm. | | ||
| | `precision` | `'single'` for single precision or `'double'` for double precision. | | ||
| | `device` | The PyTorch device to perform calculations on, either a `torch.device` object or a string (such as `'cuda'` or `'cpu'`.) If omitted, a device is chosen automatically. | | ||
|
|
There was a problem hiding this comment.
Do we also want to support the other embedding schemes available in emle-engine? Please see: https://github.com/chemle/emle-engine/blob/devel/emle/models/_emle.py#L101-L114. They have been used in some recent papers.
There was a problem hiding this comment.
I wasn't sure if there were benefits to exposing anything other than electrostatic, but if people are using the others, we can support them.
| if self.name == "emle": | ||
| modelPath = None | ||
| elif self.name == "emle-engine": | ||
| try: | ||
| modelPath = args["embeddingModelPath"] | ||
| except KeyError: | ||
| raise ValueError("For the emle-engine embedding method, an embeddingModelPath must be provided") | ||
| else: | ||
| raise ValueError(f"Unrecognized embedding name {self.name!r} for EMLE (recognized names are emle, emle-engine)") |
There was a problem hiding this comment.
I'd probably suggest simplifying this bit and leaving self.name always as emle. A modelPath can be optionally accepted if the user wants to use a bespoke EMLE model. I think this is closer to the logic used for example in the mace implementation.
There was a problem hiding this comment.
In MACE we have mace to load a custom model and mace-... for various pretrained models. I was thinking along these lines where emle-engine would be used to load a custom model with the EMLE code, while emle would give the default pretrained EMLE model, and if there were a future pretrained model like EMLE2 or whatever, it could have another name. But if that doesn't make sense for EMLE, e.g., you're planning on exposing any future models only through new method keyword arguments, etc., I could make it just emle.
| cell = state.getPeriodicBoxVectors(asNumpy=True).value_in_unit(unit.angstrom) | ||
| cellTensor = torch.tensor(cell, dtype=dtype, device=device) | ||
| else: | ||
| cellTensor = None |
There was a problem hiding this comment.
See chemle/emle-engine#91 for the support we discussed for nonperiodic systems.
| # TODO: EMLE energy values have a lot of numerical noise. Is this expected? | ||
| atol = 0.06 |
There was a problem hiding this comment.
I might have mentioned this to you already but I think 0.05 kJ/mol is about the single precision limit for Hartree energies, so it might be worth running the tests in double precision.
| 'emle-engine = openmmml.embeddings.emleembedding:EMLEEmbeddingFactory', | ||
| 'emle = openmmml.embeddings.emleembedding:EMLEEmbeddingFactory', |
There was a problem hiding this comment.
This could be made just one as I mentioned previously.
| # TODO: This script should ideally compute reference energies for EMLE without | ||
| # using OpenMM-ML or calling the EMLE model directly. |
There was a problem hiding this comment.
See the attached zip file for an updated version of this file that uses Sire-EMLE. It also contains the output, which I have confirmed matches the current expected_energy value in TestEMLEEmbedding.py
There was a problem hiding this comment.
Thanks! Is there any chance of doing periodic and non-periodic cases (the latter using the same system but with NoCutoff or CutoffNonPeriodic as the nonbondedMethod)? I had a non-periodic test also at first, then removed it when the support for the EMLE preprocessing was added. Now that the preprocessing supports periodic and non-periodic systems, I might want to add a test case back for it if possible.
|
@JMorado Regarding your comment from our other conversation:
This would make sense if we could guarantee that they would have meaningful values that actually are in use, but that is not always the case. The cutoff distance is normally in use unless a user is simulating a non-periodic system and has specified As for the non-periodic preprocessing option, I’ve seen that this was merged into the “devel” branch. Which branch is the release going to come from? I’ve been trying to test against the “devel” branch now to try this option out. I found using this branch that the EMLE model object can no longer be pickled, which breaks the OpenMM-ML interpolation feature, since it needs to clone its PythonForce which involves serializing and deserializing it. I don’t know if this was intentional or not. |
Adds support for EMLE embedding.
(Work in progress, requires #158 and needs updates, review, more testing, and possibly an EMLE-Engine release.)