Skip to content

Commit 16ba984

Browse files
committed
fix(mta): correct threading comment, model-driven embedding, link docs
- Threading: document PyTorch cached-thread-count + MKL synchronization as the reason for unconditional at::set_num_threads (replaces incorrect dual-OMP-runtime explanation) - Remove metatomic-electrostatic-embedding MDP option; charges always collected but only passed if model requests point_charges input - Improve link atom documentation comments
1 parent 2314cd5 commit 16ba984

3 files changed

Lines changed: 24 additions & 30 deletions

File tree

src/gromacs/applied_forces/metatomic/metatomic_forceprovider.cpp

Lines changed: 16 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -241,11 +241,15 @@ MetatomicForceProvider::MetatomicForceProvider(const MetatomicOptions& options,
241241
// For GPU devices, CPU overhead is minimal so we keep 1 thread to avoid
242242
// oversubscription with GROMACS threads. For CPU devices, model inference
243243
// (matmuls, convolutions) benefits from multi-threading.
244-
// Set PyTorch thread count to match GROMACS OpenMP threads.
245-
// This is critical for thread-MPI builds: PyTorch's default (all cores)
246-
// conflicts with thread-MPI's internal threading, causing incorrect
247-
// forces and simulation blow-up. For real MPI with multiple ranks,
248-
// this also prevents oversubscription.
244+
// Synchronize PyTorch's thread count with GROMACS's ntomp.
245+
//
246+
// at::set_num_threads updates PyTorch's cached thread count AND
247+
// MKL's thread pool (via mkl_set_num_threads), while GROMACS's
248+
// omp_set_num_threads only updates the OpenMP ICV. Without this
249+
// call, PyTorch/MKL may retain the init-time default (all cores).
250+
//
251+
// For real MPI, ntomp defaults to all cores for single-rank and to
252+
// the per-rank count for multi-rank, so the behavior is unchanged.
249253
{
250254
int ntomp = gmx_omp_nthreads_get(ModuleMultiThread::Default);
251255
at::set_num_threads(std::max(1, ntomp));
@@ -1166,10 +1170,13 @@ void MetatomicForceProvider::calculateForces(const ForceProviderInput& inputs, F
11661170
}
11671171
copy_mat(inputs.box_, box_);
11681172

1169-
// Link atom setup: find MTA indices for link frontier atoms and
1170-
// overwrite boundary MM atom types to hydrogen. The position
1171-
// replacement is done INSIDE the autograd graph (after torch tensor
1172-
// creation) so that forces are automatically correct via chain rule.
1173+
// Link atom setup: boundary MM atoms are replaced with hydrogen link
1174+
// atoms in the model input. Positions are overwritten here (in C++),
1175+
// then recomputed inside the autograd graph (via torch ops) so that
1176+
// forces are automatically correct via the chain rule.
1177+
//
1178+
// The user sees this as: "N embedded atoms, M of which are link atoms
1179+
// (hydrogen caps at ML/MM boundary bonds)."
11731180
if (!data_->linkFrontier.empty())
11741181
{
11751182
bool needTypesRebuild = false;

src/gromacs/applied_forces/metatomic/metatomic_options.cpp

Lines changed: 8 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,6 @@ static const std::string NON_CONSERVATIVE_TAG = "non-conservative";
8686
static const std::string VARIANT_NC_FORCES_TAG = "variant-nc-forces";
8787
static const std::string VARIANT_NC_STRESS_TAG = "variant-nc-stress";
8888
static const std::string LINK_ATOMS_TAG = "link-atoms";
89-
static const std::string ELECTROSTATIC_EMBEDDING_TAG = "electrostatic-embedding";
9089

9190
namespace
9291
{
@@ -176,8 +175,6 @@ void MetatomicOptions::initMdpTransform(IKeyValueTreeTransformRules* rules)
176175
rules, stringIdentityTransform, METATOMIC_MODULE_NAME, VARIANT_NC_STRESS_TAG);
177176
addMdpTransformFromString<bool>(
178177
rules, &fromStdString<bool>, METATOMIC_MODULE_NAME, LINK_ATOMS_TAG);
179-
addMdpTransformFromString<bool>(
180-
rules, &fromStdString<bool>, METATOMIC_MODULE_NAME, ELECTROSTATIC_EMBEDDING_TAG);
181178
}
182179

183180
void MetatomicOptions::initMdpOptions(IOptionsContainerWithSections* options)
@@ -197,7 +194,6 @@ void MetatomicOptions::initMdpOptions(IOptionsContainerWithSections* options)
197194
section.addOption(StringOption(VARIANT_NC_FORCES_TAG.c_str()).store(&params_.variantNcForces));
198195
section.addOption(StringOption(VARIANT_NC_STRESS_TAG.c_str()).store(&params_.variantNcStress));
199196
section.addOption(BooleanOption(LINK_ATOMS_TAG.c_str()).store(&params_.linkAtoms));
200-
section.addOption(BooleanOption(ELECTROSTATIC_EMBEDDING_TAG.c_str()).store(&params_.electrostaticEmbedding));
201197
}
202198

203199
void MetatomicOptions::buildMdpOutput(KeyValueTreeObjectBuilder* builder) const
@@ -235,8 +231,6 @@ void MetatomicOptions::buildMdpOutput(KeyValueTreeObjectBuilder* builder) const
235231
builder, METATOMIC_MODULE_NAME, VARIANT_NC_STRESS_TAG, params_.variantNcStress);
236232
addMdpOutputValue<bool>(
237233
builder, METATOMIC_MODULE_NAME, LINK_ATOMS_TAG, params_.linkAtoms);
238-
addMdpOutputValue<bool>(
239-
builder, METATOMIC_MODULE_NAME, ELECTROSTATIC_EMBEDDING_TAG, params_.electrostaticEmbedding);
240234
}
241235
}
242236

@@ -274,24 +268,19 @@ void MetatomicOptions::modifyTopology(gmx_mtop_t* top)
274268
return;
275269
}
276270

277-
// Collect MM charges before preprocessing (which may modify charges)
278-
if (params_.electrostaticEmbedding)
271+
// Always collect MM charges (cheap). The force provider will pass them
272+
// to the model only if the model declares point_charges in its
273+
// requested_inputs (model-driven, not user-controlled).
274+
for (const auto& molblock : top->molblock)
279275
{
280-
for (const auto& molblock : top->molblock)
276+
const auto& moltype = top->moltype[molblock.type];
277+
for (int m = 0; m < molblock.nmol; m++)
281278
{
282-
const auto& moltype = top->moltype[molblock.type];
283-
for (int m = 0; m < molblock.nmol; m++)
279+
for (int a = 0; a < moltype.atoms.nr; a++)
284280
{
285-
for (int a = 0; a < moltype.atoms.nr; a++)
286-
{
287-
params_.mmCharges_.push_back(moltype.atoms.atom[a].q);
288-
}
281+
params_.mmCharges_.push_back(moltype.atoms.atom[a].q);
289282
}
290283
}
291-
GMX_LOG(logger().info)
292-
.appendTextFormatted("Metatomic: collected %zu point charges for "
293-
"electrostatic embedding",
294-
params_.mmCharges_.size());
295284
}
296285

297286
if (params_.linkAtoms)

src/gromacs/applied_forces/metatomic/metatomic_options.h

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -105,8 +105,6 @@ struct MetatomicParameters
105105

106106
//! Enable ONIOM link atoms at cut bonds between ML and MM regions
107107
bool linkAtoms = false;
108-
//! Enable electrostatic embedding (pass MM point charges to ML model)
109-
bool electrostaticEmbedding = false;
110108

111109
std::vector<Index> mtaIndices_;
112110
std::vector<Index> mmIndices_;

0 commit comments

Comments
 (0)