Simplify and optimize coordination numbers #909

giacomofiorin · 2026-01-21T20:56:40Z

This PR contains several improvements:

Condense the computation of multiple variants of coordination numbers: all classes are now derived from coordnum with the exception of h_bond
Separate the pairlist update from the switching function computation
Skip computing the switching function if pairwise distance is too large for the given tolerance

Fixes #908

…ith no changes) In the process, also flip the outdated order of public vs. protected members.

…no changes)

…gh for given tolerance

giacomofiorin · 2026-01-24T00:41:46Z

Side note: an additional simplification I wanted to add would be to change the h_bond class so that it can handle multiple pairs (i.e. not just a single pair). The alpha variable at that point could just use a single child h_bond object instead of multiple ones (and the same could be done for the angles, at which point it's clearly way out of scope for this PR).

Inlining the functions is required to avoid the performance loss after refactoring.

HanatoK

Looks good to me! Although I need to use inline for compute_pair_coordnum to restore the performance after refactoring.

giacomofiorin · 2026-01-26T16:40:44Z

Looks good to me! Although I need to use inline for compute_pair_coordnum to restore the performance after refactoring.

Thanks! I have been removing inline from template functions because it's generally redundant. But reading a bit more, that generally happens when the template functions are fully specialized: here, we have now four nested levels of template functions calling one another...

HanatoK · 2026-01-26T20:40:07Z

src/colvarcomp_coordnums.cpp

+  if (function_type() != "selfCoordNum") {
+
+    group2 = parse_group(conf, "group2");
+    if (!group2) {
+      return error_code | COLVARS_INPUT_ERROR;
+    }
+
+    if (int atom_number = cvm::atom_group::overlap(*group1, *group2)) {
+      error_code |= cvm::error("Error: group1 and group2 share a common atom (number: " +
+                                   cvm::to_str(atom_number) + ")\n",
+                               COLVARS_INPUT_ERROR);
+    }
+
+    if (function_type() == "coordNum") {
+      get_keyval(conf, "group1CenterOnly", b_group1_center_only, group2->b_dummy);
+      get_keyval(conf, "group2CenterOnly", b_group2_center_only, group2->b_dummy);
+    }
+
+    if (function_type() == "groupCoord") {
+      // In groupCoord, these flags are hard-coded
+      b_group1_center_only = true;
+      b_group2_center_only = true;
+    }
+
+    size_t const group1_num_coords = b_group1_center_only ? 1 : group1->size();
+    size_t const group2_num_coords = b_group2_center_only ? 1 : group2->size();
+
+    num_pairs = group1_num_coords * group2_num_coords;
+


Why not using different init() implementations for each derived class of coordnum?

Although there are some sub-cases that differ, the initialization of the cutoffs, exponents and pairlist is shared.

HanatoK · 2026-01-26T21:09:36Z

src/colvarcomp_coordnums.h

+  int pairlist_freq = 100;
+
+  /// Pair list
+  std::vector<bool> pairlist;


When using pairlist, the performance degrades a lot compared to the master branch. I suspect it is because that std::vector<bool> actually use bit fields. @giacomofiorin Have you compared the performance before and after this PR?

Not yet, good to know.

My main goal was to clean up the code and condense code paths, so it should be easy to revert to a C-style array (at the price of higher memory footprint)

It's possible to change to vector<char> but it still doesn't solve the issue of performance loss. You could merge the PR as is if you don't think that the performance loss on CPU is a big issue...

We'll need more precise numbers for relevant use cases, I think.

giacomofiorin added 9 commits January 22, 2026 16:48

refactor: Move coordNum classes' definitions to a dedicated header (w…

22c34d8

…ith no changes) In the process, also flip the outdated order of public vs. protected members.

refactor: Move coordNum switching function to dedicated header (with …

7254f6b

…no changes)

cleanup: Remove unused parameter

c737a8a

cleanup: Use vector cutoff for coordNum even in the isotropic case

d04ce3a

cleanup: Inherit selfCoordNum to coordNum to reuse its init function

d24a7b1

cleanup: Remove redundant check

eeafb33

cleanup: Derive groupCoord from coordNum

0eddd69

feat: Support group1CenterOnly, consolidate cases including groupCoord

03ac89a

refactor: Make selfCoordNum use a template function rather than a macro

6cf14f5

giacomofiorin force-pushed the coordnum-template branch from 103663c to 6cf14f5 Compare January 23, 2026 02:04

giacomofiorin added 6 commits January 22, 2026 23:18

opt: Manage pairlist outside of switching function kernel

5b9f667

cleanup: Condense initialization of cutoff-related fields

ceb7e79

build: Use spiff directly when available to support out-of-tree builds

0725c86

refactor: Split computation of scaled distance and switching function

2b62240

opt: Skip computation of switching function if the distance is too hi…

64f214c

…gh for given tolerance

cleanup: Make pairlist buffer a C++ object

7eedec3

giacomofiorin marked this pull request as ready for review January 24, 2026 00:37

giacomofiorin changed the title ~~WIP: Simplify and optimize coordination numbers~~ Simplify and optimize coordination numbers Jan 24, 2026

giacomofiorin added the optimization label Jan 24, 2026

giacomofiorin requested review from HanatoK and jhenin and removed request for HanatoK January 24, 2026 00:37

perf: inline compute_pair_coordnum and switching_function

bd60850

Inlining the functions is required to avoid the performance loss after refactoring.

HanatoK approved these changes Jan 26, 2026

View reviewed changes

HanatoK reviewed Jan 26, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify and optimize coordination numbers #909

Simplify and optimize coordination numbers #909

giacomofiorin commented Jan 21, 2026 •

edited

Loading

Uh oh!

giacomofiorin commented Jan 24, 2026 •

edited

Loading

Uh oh!

HanatoK left a comment

Uh oh!

giacomofiorin commented Jan 26, 2026

Uh oh!

HanatoK Jan 26, 2026

Uh oh!

giacomofiorin Jan 26, 2026

Uh oh!

HanatoK Jan 26, 2026

Uh oh!

giacomofiorin Jan 26, 2026

Uh oh!

HanatoK Jan 26, 2026 •

edited

Loading

Uh oh!

giacomofiorin Jan 26, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Simplify and optimize coordination numbers #909

Are you sure you want to change the base?

Simplify and optimize coordination numbers #909

Conversation

giacomofiorin commented Jan 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

giacomofiorin commented Jan 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HanatoK left a comment

Choose a reason for hiding this comment

Uh oh!

giacomofiorin commented Jan 26, 2026

Uh oh!

HanatoK Jan 26, 2026

Choose a reason for hiding this comment

Uh oh!

giacomofiorin Jan 26, 2026

Choose a reason for hiding this comment

Uh oh!

HanatoK Jan 26, 2026

Choose a reason for hiding this comment

Uh oh!

giacomofiorin Jan 26, 2026

Choose a reason for hiding this comment

Uh oh!

HanatoK Jan 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

giacomofiorin Jan 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

giacomofiorin commented Jan 21, 2026 •

edited

Loading

giacomofiorin commented Jan 24, 2026 •

edited

Loading

HanatoK Jan 26, 2026 •

edited

Loading

giacomofiorin Jan 26, 2026 •

edited

Loading