Skip to content

refactor: make virial a pure static class - #490

Open
97gamjak wants to merge 9 commits into
devfrom
486-cleanup-virial-class
Open

refactor: make virial a pure static class#490
97gamjak wants to merge 9 commits into
devfrom
486-cleanup-virial-class

Conversation

@97gamjak

@97gamjak 97gamjak commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator

closes #486

@97gamjak
97gamjak requested review from ape33 and galjos August 11, 2026 21:06
@pq-perf-bot

pq-perf-bot Bot commented Aug 11, 2026

Copy link
Copy Markdown

⚡ Performance (instruction count) — 🎉 improvement: virial -12.79%

per-benchmark breakdown
benchmark base Ir PR Ir Δ
bondedForces 38.24M 38.24M -0.00%
boxTransforms 10.67M 10.67M +0.00%
constraints 10.92M 10.91M -0.04%
coulombKernel 5.80M 5.80M +0.00%
forceKernel 14.16M 14.16M -0.00%
integrator 32.44M 32.44M -0.00%
interWater 1681.44M 1681.40M -0.00%
kinetics 8.55M 8.55M -0.01%
linearAlgebra 2.08M 2.08M +0.00%
nonCoulombPairs 5.48M 5.48M +0.00%
shiftVector 5.81M 5.81M +0.00%
virial 10.44M 9.10M -12.79% 🎉

Deterministic callgrind instruction counts vs the base branch; gated at ±2%. Not wall-clock.

galjos
galjos previously approved these changes Aug 11, 2026
Comment thread tests/include/virial/testVirial.hpp
Comment thread tests/include/virial/testVirial.hpp Outdated
Comment thread tests/src/virial/testVirial.cpp
@codecov

codecov Bot commented Aug 11, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 76.66667% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.19%. Comparing base (1ace0e9) to head (e4698c6).
⚠️ Report is 14 commits behind head on dev.

Files with missing lines Patch % Lines
src/virial/virial.cpp 75.86% 7 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #490      +/-   ##
==========================================
+ Coverage   91.13%   91.19%   +0.05%     
==========================================
  Files         319      316       -3     
  Lines       14534    14515      -19     
  Branches     4817     4810       -7     
==========================================
- Hits        13246    13237       -9     
+ Misses       1202     1192      -10     
  Partials       86       86              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions
github-actions Bot dismissed galjos’s stale review August 12, 2026 06:10

Approval dismissed: new commits contain changes beyond a merge-up from dev.

@galjos
galjos self-requested a review August 12, 2026 07:01
@galjos

galjos commented Aug 12, 2026

Copy link
Copy Markdown
Member

@97gamjak

⚡ Performance (instruction count) — 🎉 improvement: virial -12.78%

because you removed virial timing instrumentation. Should be fix the perf bench to account for this?

@ape33 ape33 linked an issue Aug 12, 2026 that may be closed by this pull request
Comment thread src/virial/virial.cpp Outdated
Comment thread src/virial/virial.cpp Outdated
Comment thread src/engine/qmmmMDEngine.cpp Outdated
Comment thread src/virial/virial.cpp Outdated
Comment thread src/virial/virial.cpp Outdated
Comment thread include/virial/virial.hpp Outdated
Comment thread src/virial/virial.cpp Outdated
const auto centerOfMass = molecule.getCenterOfMass();
const size_t numberOfAtoms = molecule.getNumberOfAtoms();

for (size_t i = 0; i < numberOfAtoms; ++i)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use a range-based for loop here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

cleanup virial class

3 participants