Skip to content

Simplify and optimize coordination numbers#909

Open
giacomofiorin wants to merge 23 commits intomasterfrom
coordnum-template
Open

Simplify and optimize coordination numbers#909
giacomofiorin wants to merge 23 commits intomasterfrom
coordnum-template

Conversation

@giacomofiorin
Copy link
Member

@giacomofiorin giacomofiorin commented Jan 21, 2026

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
  • Optionally, allow using an inline function to compute PBC distances instead of a virtual function

Fixes #908

@giacomofiorin giacomofiorin marked this pull request as ready for review January 24, 2026 00:37
@giacomofiorin giacomofiorin changed the title WIP: Simplify and optimize coordination numbers Simplify and optimize coordination numbers Jan 24, 2026
@giacomofiorin giacomofiorin requested review from HanatoK and jhenin and removed request for HanatoK January 24, 2026 00:37
@giacomofiorin
Copy link
Member Author

giacomofiorin commented Jan 24, 2026

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).

Copy link
Member

@HanatoK HanatoK left a comment

Choose a reason for hiding this comment

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

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

@giacomofiorin
Copy link
Member Author

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...

@giacomofiorin giacomofiorin force-pushed the coordnum-template branch 2 times, most recently from 49c5b6b to 26a372a Compare January 28, 2026 19:46
@giacomofiorin
Copy link
Member Author

giacomofiorin commented Jan 30, 2026

In the latest commits, I added partial support for skipping the virtual function (part of which could cause the 22% performance loss in #911).

I have not tested yet the code in NAMD: by default, the current PBC function is used.

@HanatoK
Copy link
Member

HanatoK commented Jan 30, 2026

In the latest commits, I added partial support for skipping the virtual function (part of which could cause the 22% performance loss in #911).

I have not tested yet the code in NAMD: by default, the current PBC function is used.

I just tested with the CUDAGM interface, and using useInternalPBC on is indeed much faster!

@giacomofiorin
Copy link
Member Author

Commit df18a13 was cherry-picked from #913, but I also amended its message to reflect that the bug was not being triggered yet (I have just now pushed the same commit to master).

I had previously added the same change in the integrate_cudagm branch, but it's more complex there and not suitable for backporting. I'll now rebase the integrate_cudagm branch onto the current master and open it as a draft PR.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Incorrect number of pairs and doc error in selfCoordNum

2 participants