Add batch calculations for energies and forces#2867
Open
ghutchis wants to merge 1 commit into
Open
Conversation
Allows re-calculating E and forces for all conformers Uses batch calculations (and chunks) for efficiency with ML Signed-off-by: Geoff Hutchison <geoff.hutchison@gmail.com>
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
WalkthroughThis PR adds batch-processing support for energy and gradient evaluation across multiple coordinate sets. It introduces a new abstract interface with chunked computation, Qt threading infrastructure, binary protocol support, and UI integration to enable per-conformer energy/force calculations stored in molecule data. ChangesBatch Energy and Gradient Evaluation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
avogadro/core/molecule.cpp (4)
183-253:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCopy assignment must preserve
m_coordinate3dIndex.The copy assignment operator copies
m_coordinates3dbut notm_coordinate3dIndex, causing the assigned-to molecule to retain its old index instead of adopting the source's active coordinate.📋 Proposed fix
Add after line 200:
m_coordinates3d = other.m_coordinates3d; + m_coordinate3dIndex = other.m_coordinate3dIndex; m_velocities = other.m_velocities;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@avogadro/core/molecule.cpp` around lines 183 - 253, The copy assignment (Molecule::operator=) copies m_coordinates3d but forgets to update the active coordinate index; set m_coordinate3dIndex = other.m_coordinate3dIndex (or -1/null-equivalent if other has none) immediately after copying m_coordinates3d so the assigned object adopts the source's active 3D coordinate index; locate this in the body of Molecule::operator= near where m_coordinates3d is assigned.
144-181:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMove constructor must preserve
m_coordinate3dIndex.The move constructor does not transfer
m_coordinate3dIndex, causing the moved-to molecule to incorrectly reset to index0.📋 Proposed fix
Add to the member initializer list after line 155:
m_coordinates3d(other.m_coordinates3d), m_velocities(other.m_velocities), + m_coordinate3dIndex(other.m_coordinate3dIndex), m_timesteps(other.m_timesteps), m_hybridizations(other.m_hybridizations),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@avogadro/core/molecule.cpp` around lines 144 - 181, The move constructor Molecule::Molecule(Molecule&& other) noexcept currently omits transferring m_coordinate3dIndex so the moved-to object resets to 0; fix it by adding m_coordinate3dIndex(std::exchange(other.m_coordinate3dIndex, 0)) into the member initializer list (e.g., alongside m_coordinates3d/m_velocities) so the index value is moved and the source is reset.
37-82:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCopy constructor must preserve
m_coordinate3dIndex.The copy constructor copies
m_coordinates3dbut does not copym_coordinate3dIndex. When copying a molecule with a non-zero active coordinate index, the copy will incorrectly reset to index0, causing the wrong conformer to be active.📋 Proposed fix
Add to the member initializer list after line 47:
m_coordinates3d(other.m_coordinates3d), m_velocities(other.m_velocities), + m_coordinate3dIndex(other.m_coordinate3dIndex), m_timesteps(other.m_timesteps), m_hybridizations(other.m_hybridizations),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@avogadro/core/molecule.cpp` around lines 37 - 82, The copy constructor Molecule::Molecule currently copies m_coordinates3d but fails to copy m_coordinate3dIndex, which resets the active conformer; update the member initializer list in Molecule::Molecule to copy m_coordinate3dIndex from other (i.e., add m_coordinate3dIndex(other.m_coordinate3dIndex) alongside m_coordinates3d(other.m_coordinates3d)) so the active 3D coordinate index is preserved in the new object.
255-316:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMove assignment must preserve
m_coordinate3dIndex.The move assignment operator does not transfer
m_coordinate3dIndex, leaving the assigned-to molecule with its old index instead of the source's active coordinate.📋 Proposed fix
Add after line 272:
m_coordinates3d = other.m_coordinates3d; + m_coordinate3dIndex = other.m_coordinate3dIndex; m_velocities = other.m_velocities;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@avogadro/core/molecule.cpp` around lines 255 - 316, The move-assignment operator Molecule::operator=(Molecule&& other) fails to transfer the active 3D coordinate index, so the target keeps its previous m_coordinate3dIndex; update the move operator to assign m_coordinate3dIndex = other.m_coordinate3dIndex (and then reset other.m_coordinate3dIndex if desired) right after transferring m_coordinates3d (or anywhere before returning) so the moved-from molecule's active coordinate state is preserved in the destination.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@avogadro/qtplugins/forcefield/forcefield.cpp`:
- Around line 761-770: The Eigen vector x created in the loop (Eigen::VectorXd
x(3 * n)) can leave tail entries uninitialized when set.size() < n; before
filling positions from m_molecule->coordinate3d(c) you should initialize x to
zeros (or another deterministic default) so any atoms missing from Vector3 set
are deterministic, then copy set[i].x/y/z for i < set.size(), and finally
push_back(x) as before; reference symbols: m_molecule->coordinate3d, Vector3,
Eigen::VectorXd x, n, set.size(), coords.push_back(x).
- Around line 849-906: onBatchDone currently writes whatever energies/gradients
it receives into molecule-wide "energies"/"forces" and per-conformer matrices,
which can persist partial/cancelled results; change it to first check that the
returned counts match the molecule's coordinate3dCount() (use
m_molecule->coordinate3dCount()) and only replace the global arrays and loop
over conformers when energies.size() == coordCount and gradients.size() ==
coordCount; for safety, when writing per-conformer force matrices use coordCount
bounds (or skip excess entries) and when updating the displayed force vectors
ensure the activeIndex < gradients.size() and each gradient vector has length >=
3*n before reading from g; if counts mismatch, avoid overwriting the
molecule-wide "energies"/"forces" and only update ephemeral UI (setForceVectors)
for the active conformer when its gradient is complete.
- Around line 322-325: In Forcefield::setMolecule() you must disconnect the
previous m_molecule sender before connecting the new one to avoid stale signals;
call disconnect(m_molecule, SIGNAL(changed(unsigned int)), this,
SLOT(updateActions())) (guarded by a non-null m_molecule) prior to assigning the
new m_molecule, then connect the new m_molecule to updateActions() and call
updateActions() as currently done so the batch-action enable state is driven
only by the currently bound molecule.
- Around line 801-842: The current code copies the entire coords vector into the
queued QMetaObject::invokeMethod (via the calculatorReady lambda and Q_ARG),
causing a deep copy before the worker can chunk; change the handoff to avoid
copying by creating a shared pointer (e.g.,
std::shared_ptr<std::vector<Eigen::VectorXd>>) for coords, capture/move that
shared_ptr into the calculatorReady lambda, and update
CalcWorker::runEvaluateBatch signature to accept a
std::shared_ptr<std::vector<Eigen::VectorXd>> (or const std::shared_ptr<...>&)
so you pass the shared_ptr via Q_ARG and the worker can chunk without
duplicating element storage; adjust all call sites and the lambda to use the new
shared_ptr parameter and ensure m_worker invocation uses the shared_ptr instead
of the raw coords vector.
In `@avogadro/qtplugins/forcefield/scriptenergy.cpp`:
- Around line 620-664: The code currently computes dataDoubles by truncating
bytes/sizeof(double) which allows trailing garbage; update the parsing to
validate exact byte counts instead of truncated counts: compute actualDataBytes
= payloadEnd - data and verify actualDataBytes % sizeof(double) == 0, then set
dataDoubles = actualDataBytes / sizeof(double); additionally check that
dataDoubles equals the expected counts used in the gradient branch (frameSize *
batchSize) and in the energy-only branch (batchSize) before calling readDoubles
(references: the dataDoubles calculation, the readDoubles lambda, the gradient
branch using frameSize and batchSize, and the final energy-only branch that
calls readDoubles). Ensure any early returns occur if sizes mismatch so leftover
bytes are rejected.
In `@avogadro/qtplugins/plotconformer/plotconformer.cpp`:
- Around line 235-236: The y-axis label is misleading: update the yTitle
assignment in plotconformer.cpp (the yTitle variable currently set to
tr("Forces")) to reflect the actual data meaning by changing it to tr("RMS
Gradient") so the plotted series that uses data("forces") is correctly labeled;
locate the yTitle assignment near where the plot is configured and replace the
string literal accordingly (use tr(...) to keep translations).
---
Outside diff comments:
In `@avogadro/core/molecule.cpp`:
- Around line 183-253: The copy assignment (Molecule::operator=) copies
m_coordinates3d but forgets to update the active coordinate index; set
m_coordinate3dIndex = other.m_coordinate3dIndex (or -1/null-equivalent if other
has none) immediately after copying m_coordinates3d so the assigned object
adopts the source's active 3D coordinate index; locate this in the body of
Molecule::operator= near where m_coordinates3d is assigned.
- Around line 144-181: The move constructor Molecule::Molecule(Molecule&& other)
noexcept currently omits transferring m_coordinate3dIndex so the moved-to object
resets to 0; fix it by adding
m_coordinate3dIndex(std::exchange(other.m_coordinate3dIndex, 0)) into the member
initializer list (e.g., alongside m_coordinates3d/m_velocities) so the index
value is moved and the source is reset.
- Around line 37-82: The copy constructor Molecule::Molecule currently copies
m_coordinates3d but fails to copy m_coordinate3dIndex, which resets the active
conformer; update the member initializer list in Molecule::Molecule to copy
m_coordinate3dIndex from other (i.e., add
m_coordinate3dIndex(other.m_coordinate3dIndex) alongside
m_coordinates3d(other.m_coordinates3d)) so the active 3D coordinate index is
preserved in the new object.
- Around line 255-316: The move-assignment operator
Molecule::operator=(Molecule&& other) fails to transfer the active 3D coordinate
index, so the target keeps its previous m_coordinate3dIndex; update the move
operator to assign m_coordinate3dIndex = other.m_coordinate3dIndex (and then
reset other.m_coordinate3dIndex if desired) right after transferring
m_coordinates3d (or anywhere before returning) so the moved-from molecule's
active coordinate state is preserved in the destination.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5f7d696b-cfa8-4e8b-883c-7b8182d76116
📒 Files selected for processing (14)
avogadro/calc/energycalculator.cppavogadro/calc/energycalculator.havogadro/core/avogadrocore.havogadro/core/molecule.cppavogadro/core/molecule.havogadro/qtgui/calcworker.cppavogadro/qtgui/calcworker.havogadro/qtplugins/forcefield/forcefield.cppavogadro/qtplugins/forcefield/forcefield.havogadro/qtplugins/forcefield/scriptenergy.cppavogadro/qtplugins/forcefield/scriptenergy.havogadro/qtplugins/plotconformer/plotconformer.cppavogadro/quantumio/mopacaux.cpptests/calc/energycalculatortest.cpp
Comment on lines
+322
to
+325
| // Refresh action enable-state (e.g. batch actions) when the molecule | ||
| // changes - conformers may be added or removed. | ||
| connect(m_molecule, SIGNAL(changed(unsigned int)), SLOT(updateActions())); | ||
| updateActions(); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C2 'void Forcefield::setMolecule|connect\(m_molecule, SIGNAL\(changed\(unsigned int\)\), SLOT\(updateActions\(\)\)\)|disconnect\(m_molecule' avogadro/qtplugins/forcefield/forcefield.cppRepository: OpenChemistry/avogadrolibs
Length of output: 433
Disconnect the previous molecule before wiring changed() in Forcefield::setMolecule()
Forcefield::setMolecule() connects m_molecule to updateActions() but never disconnects the previous m_molecule. When rebinding to a different QtGui::Molecule, the old sender can still trigger updateActions(), potentially updating the batch-action enable state from the wrong molecule.
Suggested fix
void Forcefield::setMolecule(QtGui::Molecule* mol)
{
if (mol == nullptr || m_molecule == mol)
return;
+ if (m_molecule != nullptr) {
+ disconnect(m_molecule, SIGNAL(changed(unsigned int)),
+ this, SLOT(updateActions()));
+ }
+
m_molecule = mol;
// Refresh action enable-state (e.g. batch actions) when the molecule
// changes - conformers may be added or removed.
connect(m_molecule, SIGNAL(changed(unsigned int)), SLOT(updateActions()));
updateActions();
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@avogadro/qtplugins/forcefield/forcefield.cpp` around lines 322 - 325, In
Forcefield::setMolecule() you must disconnect the previous m_molecule sender
before connecting the new one to avoid stale signals; call
disconnect(m_molecule, SIGNAL(changed(unsigned int)), this,
SLOT(updateActions())) (guarded by a non-null m_molecule) prior to assigning the
new m_molecule, then connect the new m_molecule to updateActions() and call
updateActions() as currently done so the batch-action enable state is driven
only by the currently bound molecule.
Comment on lines
+761
to
+770
| for (size_t c = 0; c < count; ++c) { | ||
| // coordinate3d() returns a copy and does not change the displayed set. | ||
| Core::Array<Vector3> set = m_molecule->coordinate3d(c); | ||
| Eigen::VectorXd x(3 * n); | ||
| for (size_t i = 0; i < n && i < set.size(); ++i) { | ||
| x[3 * i] = set[i].x(); | ||
| x[3 * i + 1] = set[i].y(); | ||
| x[3 * i + 2] = set[i].z(); | ||
| } | ||
| coords.push_back(x); |
Contributor
There was a problem hiding this comment.
Initialize missing coordinates before enqueuing a frame.
x is default-initialized here, but the loop only fills entries covered by set.size(). If a conformer/frame is shorter than atomCount(), the tail stays uninitialized and gets sent to the calculator, which makes batch energies/forces nondeterministic.
Suggested fix
- Eigen::VectorXd x(3 * n);
+ Eigen::VectorXd x = Eigen::VectorXd::Zero(3 * n);
for (size_t i = 0; i < n && i < set.size(); ++i) {
x[3 * i] = set[i].x();
x[3 * i + 1] = set[i].y();
x[3 * i + 2] = set[i].z();
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@avogadro/qtplugins/forcefield/forcefield.cpp` around lines 761 - 770, The
Eigen vector x created in the loop (Eigen::VectorXd x(3 * n)) can leave tail
entries uninitialized when set.size() < n; before filling positions from
m_molecule->coordinate3d(c) you should initialize x to zeros (or another
deterministic default) so any atoms missing from Vector3 set are deterministic,
then copy set[i].x/y/z for i < set.size(), and finally push_back(x) as before;
reference symbols: m_molecule->coordinate3d, Vector3, Eigen::VectorXd x, n,
set.size(), coords.push_back(x).
Comment on lines
+801
to
+842
| std::vector<Eigen::VectorXd> coords = gatherCoordinateSets(); | ||
| if (coords.empty()) | ||
| return; | ||
|
|
||
| m_batchGradient = computeGradient; | ||
|
|
||
| startWorker(); | ||
| m_batchRunning = true; | ||
|
|
||
| const int total = static_cast<int>(coords.size()); | ||
| m_progressDialog = | ||
| new QProgressDialog(qobject_cast<QWidget*>(this->parent())); | ||
| m_progressDialog->setWindowTitle(computeGradient | ||
| ? tr("Forces (All Conformers)") | ||
| : tr("Energies (All Conformers)")); | ||
| m_progressDialog->setRange(0, total); | ||
| m_progressDialog->setWindowModality(Qt::WindowModal); | ||
| m_progressDialog->setMinimumDuration(0); | ||
| m_progressDialog->show(); | ||
|
|
||
| connect(m_progressDialog, &QProgressDialog::canceled, this, [this]() { | ||
| if (m_worker) | ||
| m_worker->cancel(); | ||
| cleanupWorker(); | ||
| m_batchRunning = false; | ||
| }); | ||
|
|
||
| connect(m_worker, &QtGui::CalcWorker::batchProgress, this, | ||
| [this](int done, int totalSets) { | ||
| if (m_progressDialog) { | ||
| m_progressDialog->setRange(0, totalSets); | ||
| m_progressDialog->setValue(done); | ||
| } | ||
| }); | ||
|
|
||
| connect(m_worker, &QtGui::CalcWorker::calculatorReady, this, | ||
| [this, coords, computeGradient]() { | ||
| QMetaObject::invokeMethod( | ||
| m_worker, "runEvaluateBatch", Qt::QueuedConnection, | ||
| Q_ARG(std::vector<Eigen::VectorXd>, coords), | ||
| Q_ARG(bool, computeGradient), Q_ARG(int, 0)); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C2 'gatherCoordinateSets|calculatorReady|\[this, coords, computeGradient\]|runEvaluateBatch|Q_ARG\(std::vector<Eigen::VectorXd>, coords\)' avogadro/qtplugins/forcefield/forcefield.cpp avogadro/qtgui/calcworker.hRepository: OpenChemistry/avogadrolibs
Length of output: 3640
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n --hidden --no-ignore-vcs "runEvaluateBatch\\(" avogadro/qtgui/calcworker.* avogadro/qtplugins/forcefield/forcefield.* avogadro -SRepository: OpenChemistry/avogadrolibs
Length of output: 482
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '80,190p' avogadro/qtgui/calcworker.cpp | nl -baRepository: OpenChemistry/avogadrolibs
Length of output: 113
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat -n avogadro/qtgui/calcworker.cpp | sed -n '80,190p'Repository: OpenChemistry/avogadrolibs
Length of output: 4031
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '760,860p' avogadro/qtplugins/forcefield/forcefield.cpp | cat -nRepository: OpenChemistry/avogadrolibs
Length of output: 3606
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n --hidden "gradientBatch\\(|valueBatch\\(" avogadro/qtgui avogadro/qtplugins avogadro -SRepository: OpenChemistry/avogadrolibs
Length of output: 2800
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' avogadro/calc/energycalculator.h | cat -nRepository: OpenChemistry/avogadrolibs
Length of output: 8530
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,200p' avogadro/calc/energycalculator.cpp | cat -nRepository: OpenChemistry/avogadrolibs
Length of output: 7543
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "Q_DECLARE_METATYPE\\(std::vector<Eigen::VectorXd>|qRegisterMetaType<.*std::vector<Eigen::VectorXd>|DECLARE_METATYPE\\(std::shared_ptr|Q_DECLARE_METATYPE\\(std::shared_ptr" avogadro/qtgui avogadro/qtplugins avogadro -SRepository: OpenChemistry/avogadrolibs
Length of output: 398
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,80p' avogadro/qtgui/calcworker.cpp | cat -nRepository: OpenChemistry/avogadrolibs
Length of output: 3382
Avoid deep-copying the full coordinate batch before chunking
In avogadro/qtplugins/forcefield/forcefield.cpp (lines 801-842), coords is captured by value in the calculatorReady lambda and then passed through the queued QMetaObject::invokeMethod as Q_ARG(std::vector<Eigen::VectorXd>, coords) to CalcWorker::runEvaluateBatch(std::vector<Eigen::VectorXd> coordsList, ...). That duplicates the entire batch before runEvaluateBatch even starts chunking (and it additionally creates a per-chunk chunk vector). On large workloads this can significantly increase peak memory and undermine the intended memory-bounded behavior.
Fix: avoid deep-copying the whole coords vector before the worker runs (e.g., move it into the queued call or pass a shared/owned handle and adjust runEvaluateBatch accordingly).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@avogadro/qtplugins/forcefield/forcefield.cpp` around lines 801 - 842, The
current code copies the entire coords vector into the queued
QMetaObject::invokeMethod (via the calculatorReady lambda and Q_ARG), causing a
deep copy before the worker can chunk; change the handoff to avoid copying by
creating a shared pointer (e.g., std::shared_ptr<std::vector<Eigen::VectorXd>>)
for coords, capture/move that shared_ptr into the calculatorReady lambda, and
update CalcWorker::runEvaluateBatch signature to accept a
std::shared_ptr<std::vector<Eigen::VectorXd>> (or const std::shared_ptr<...>&)
so you pass the shared_ptr via Q_ARG and the worker can chunk without
duplicating element storage; adjust all call sites and the lambda to use the new
shared_ptr parameter and ensure m_worker invocation uses the shared_ptr instead
of the raw coords vector.
Comment on lines
+849
to
+906
| void Forcefield::onBatchDone(std::vector<double> energies, | ||
| std::vector<Eigen::VectorXd> gradients) | ||
| { | ||
| m_batchRunning = false; | ||
|
|
||
| if (m_molecule == nullptr) { | ||
| cleanupWorker(); | ||
| return; | ||
| } | ||
|
|
||
| const auto n = m_molecule->atomCount(); | ||
|
|
||
| // Store one energy per coordinate set (read by the conformer plot). | ||
| if (!energies.empty()) | ||
| m_molecule->setData("energies", Core::Variant(energies)); | ||
|
|
||
| // Store forces two ways: | ||
| // * data("forces") - the RMS gradient (|g| / sqrt(3N)) per coordinate | ||
| // set, the scalar convention shared with the readers and read by the | ||
| // conformer plot (parallels data("energies")). | ||
| // * conformerProperties("forces") - the full (N x 3) force matrix per | ||
| // coordinate set, for richer per-atom use. | ||
| if (!gradients.empty()) { | ||
| std::vector<double> rmsGradients; | ||
| rmsGradients.reserve(gradients.size()); | ||
| for (size_t c = 0; c < gradients.size(); ++c) { | ||
| const Eigen::VectorXd& g = gradients[c]; | ||
| rmsGradients.push_back( | ||
| g.size() > 0 ? g.norm() / std::sqrt(static_cast<double>(g.size())) | ||
| : 0.0); | ||
|
|
||
| MatrixX forceMat(static_cast<Eigen::Index>(n), 3); | ||
| forceMat.setZero(); | ||
| for (size_t i = 0; i < n && 3 * i + 2 < static_cast<size_t>(g.size()); | ||
| ++i) { | ||
| forceMat(static_cast<Eigen::Index>(i), 0) = -g[3 * i]; | ||
| forceMat(static_cast<Eigen::Index>(i), 1) = -g[3 * i + 1]; | ||
| forceMat(static_cast<Eigen::Index>(i), 2) = -g[3 * i + 2]; | ||
| } | ||
| m_molecule->conformerProperties().setMatrix( | ||
| "forces", static_cast<Index>(c), forceMat); | ||
| } | ||
| m_molecule->setData("forces", Core::Variant(rmsGradients)); | ||
|
|
||
| // Update the displayed force vectors for the currently shown conformer. | ||
| const size_t activeIndex = static_cast<size_t>(m_molecule->coordinate3d()); | ||
| if (activeIndex < gradients.size()) { | ||
| Core::Array<Vector3> forceVecs(n); | ||
| const Eigen::VectorXd& g = gradients[activeIndex]; | ||
| for (size_t i = 0; i < n && 3 * i + 2 < static_cast<size_t>(g.size()); | ||
| ++i) | ||
| forceVecs[i] = Vector3(-g[3 * i], -g[3 * i + 1], -g[3 * i + 2]); | ||
| m_molecule->setForceVectors(forceVecs); | ||
| } | ||
| } | ||
|
|
||
| Molecule::MoleculeChanges changes = Molecule::Atoms | Molecule::Modified; | ||
| m_molecule->emitChanged(changes); |
Contributor
There was a problem hiding this comment.
Don’t persist partial batch results as full conformer data.
evaluateBatchFinished() explicitly allows partial vectors after cancellation, but onBatchDone() writes whatever it receives into molecule-wide "energies" / "forces" storage and per-conformer force matrices without checking that the result count still matches coordinate3dCount(). A mid-run cancel will therefore leave truncated arrays and stale conformer properties behind.
Suggested guard
const auto n = m_molecule->atomCount();
+ const auto expected = static_cast<size_t>(m_molecule->coordinate3dCount());
// Store one energy per coordinate set (read by the conformer plot).
- if (!energies.empty())
+ if (!energies.empty() && energies.size() == expected)
m_molecule->setData("energies", Core::Variant(energies));
// Store forces two ways:
// * data("forces") - the RMS gradient (|g| / sqrt(3N)) per coordinate
// set, the scalar convention shared with the readers and read by the
// conformer plot (parallels data("energies")).
// * conformerProperties("forces") - the full (N x 3) force matrix per
// coordinate set, for richer per-atom use.
- if (!gradients.empty()) {
+ if (!gradients.empty() && gradients.size() == expected) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@avogadro/qtplugins/forcefield/forcefield.cpp` around lines 849 - 906,
onBatchDone currently writes whatever energies/gradients it receives into
molecule-wide "energies"/"forces" and per-conformer matrices, which can persist
partial/cancelled results; change it to first check that the returned counts
match the molecule's coordinate3dCount() (use m_molecule->coordinate3dCount())
and only replace the global arrays and loop over conformers when energies.size()
== coordCount and gradients.size() == coordCount; for safety, when writing
per-conformer force matrices use coordCount bounds (or skip excess entries) and
when updating the displayed force vectors ensure the activeIndex <
gradients.size() and each gradient vector has length >= 3*n before reading from
g; if counts mismatch, avoid overwriting the molecule-wide "energies"/"forces"
and only update ephemeral UI (setForceVectors) for the active conformer when its
gradient is complete.
Comment on lines
+620
to
+664
| const char* data = payload + sizeof(quint32); | ||
| const Eigen::Index dataDoubles = | ||
| static_cast<Eigen::Index>(payloadEnd - data) / | ||
| static_cast<Eigen::Index>(sizeof(double)); | ||
|
|
||
| const auto readDoubles = [](const char* src, const char* srcEnd, double* dest, | ||
| Eigen::Index count) -> bool { | ||
| const auto byteCount = count * static_cast<Eigen::Index>(sizeof(double)); | ||
| if (srcEnd - src < byteCount) | ||
| return false; | ||
| if (QSysInfo::ByteOrder == QSysInfo::LittleEndian) { | ||
| std::memcpy(dest, src, byteCount); | ||
| } else { | ||
| for (Eigen::Index i = 0; i < count; ++i) { | ||
| if (!readFloat64LE(src + i * sizeof(double), srcEnd, dest[i])) | ||
| return false; | ||
| } | ||
| } | ||
| return true; | ||
| }; | ||
|
|
||
| if ((requestFlags & FLAG_REQUEST_GRADIENT) != 0) { | ||
| if (grads == nullptr) | ||
| return false; | ||
| const Eigen::Index frameSize = static_cast<Eigen::Index>(atomCount) * 3; | ||
| if (dataDoubles != frameSize * static_cast<Eigen::Index>(batchSize)) | ||
| return false; | ||
| grads->assign(batchSize, Eigen::VectorXd::Zero(frameSize)); | ||
| const char* cursor = data; | ||
| for (quint32 b = 0; b < batchSize; ++b) { | ||
| if (!readDoubles(cursor, payloadEnd, (*grads)[b].data(), frameSize)) | ||
| return false; | ||
| cursor += frameSize * sizeof(double); | ||
| } | ||
| return true; | ||
| } | ||
|
|
||
| // energy-only batch response | ||
| if (energies == nullptr) | ||
| return false; | ||
| if (dataDoubles != static_cast<Eigen::Index>(batchSize)) | ||
| return false; | ||
| energies->assign(batchSize, 0.0); | ||
| return readDoubles(data, payloadEnd, energies->data(), | ||
| static_cast<Eigen::Index>(batchSize)); |
Contributor
There was a problem hiding this comment.
Reject malformed batch payloads with leftover bytes.
dataDoubles truncates with integer division here, so a payload whose data section is N * sizeof(double) + k bytes still parses successfully when k < 8. That silently accepts malformed script responses and ignores trailing garbage.
Suggested fix
const char* data = payload + sizeof(quint32);
+ if (((payloadEnd - data) % static_cast<ptrdiff_t>(sizeof(double))) != 0)
+ return false;
const Eigen::Index dataDoubles =
static_cast<Eigen::Index>(payloadEnd - data) /
static_cast<Eigen::Index>(sizeof(double));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@avogadro/qtplugins/forcefield/scriptenergy.cpp` around lines 620 - 664, The
code currently computes dataDoubles by truncating bytes/sizeof(double) which
allows trailing garbage; update the parsing to validate exact byte counts
instead of truncated counts: compute actualDataBytes = payloadEnd - data and
verify actualDataBytes % sizeof(double) == 0, then set dataDoubles =
actualDataBytes / sizeof(double); additionally check that dataDoubles equals the
expected counts used in the gradient branch (frameSize * batchSize) and in the
energy-only branch (batchSize) before calling readDoubles (references: the
dataDoubles calculation, the readDoubles lambda, the gradient branch using
frameSize and batchSize, and the final energy-only branch that calls
readDoubles). Ensure any early returns occur if sizes mismatch so leftover bytes
are rejected.
Comment on lines
235
to
+236
| // TODO: Add units | ||
| yTitle = tr("Forces (N)"); | ||
| yTitle = tr("Forces"); |
Contributor
There was a problem hiding this comment.
Label this series as RMS Gradient, not generic force.
Line 236 still names the plotted scalar as Forces, but the shared contract stores data("forces") as an RMS gradient per conformer. That is less wrong than Forces (N), but it is still misleading for users reading the chart.
Suggested fix
- // TODO: Add units
- yTitle = tr("Forces");
+ // TODO: Add units
+ yTitle = tr("RMS Gradient");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@avogadro/qtplugins/plotconformer/plotconformer.cpp` around lines 235 - 236,
The y-axis label is misleading: update the yTitle assignment in
plotconformer.cpp (the yTitle variable currently set to tr("Forces")) to reflect
the actual data meaning by changing it to tr("RMS Gradient") so the plotted
series that uses data("forces") is correctly labeled; locate the yTitle
assignment near where the plot is configured and replace the string literal
accordingly (use tr(...) to keep translations).
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
1 participant
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.
Allows re-calculating E and forces for all conformers Uses batch calculations (and chunks) for efficiency with ML
Developer Certificate of Origin
Version 1.1
Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129
Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.
Developer's Certificate of Origin 1.1
By making a contribution to this project, I certify that:
(a) The contribution was created in whole or in part by me and I
have the right to submit it under the open source license
indicated in the file; or
(b) The contribution is based upon previous work that, to the best
of my knowledge, is covered under an appropriate open source
license and I have the right under that license to submit that
work with modifications, whether created in whole or in part
by me, under the same open source license (unless I am
permitted to submit under a different license), as indicated
in the file; or
(c) The contribution was provided directly to me by some other
person who certified (a), (b) or (c) and I have not modified
it.
(d) I understand and agree that this project and the contribution
are public and that a record of the contribution (including all
personal information I submit with it, including my sign-off) is
maintained indefinitely and may be redistributed consistent with
this project or the open source license(s) involved.
Summary by CodeRabbit
New Features
Tests