refactoring: Create class for coordination number calculation #43

Thomas3R · 2025-06-12T11:25:08Z

Since some methods use different counting functions or parameters for coordination number calculations, I refactored the ncoord functionalities into a new base class, NCoordBase. This design allows for easy input of new parameters and the creation of derived classes with customized counting functions.

Additionally, cn and dcndr have been moved to be member variables of NCoordBase.

…ChargeModel class

Copilot

Pull Request Overview

This PR refactors the coordination number calculations by introducing an abstract NCoordBase class and a concrete NCoordErf subclass, and migrates existing free-function usage to member calls on these new types. It also promotes cn and dcndr to instance fields and updates tests and downstream modules to use the new API.

Extracted ncoord logic into NCoordBase/NCoordErf and moved gradients and counts into members
Updated test suites to instantiate NCoordErf and call its methods
Rewired EEQ and dispersion code to use the new class instead of standalone functions

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
test/test_ncoord.cpp	Switched test to use `NCoordErf`, allocated `cn` on member
test/test_ghost.cpp	Same update for ghost‐atom coordination number tests
src/dftd_ncoord.cpp	Implemented `NCoordBase` constructor, methods, and moved free functions inside class
include/dftd_ncoord.h	Declared `NCoordBase` and `NCoordErf` APIs
src/dftd_eeq.cpp	Replaced free‐function `get_ncoord_erf` calls with `NCoordErf` member calls
src/dftd_dispersion.cpp	Updated dispersion routine to use `NCoordErf` members
include/qceeq_param.h	Added EEQ parameter arrays

Comments suppressed due to low confidence (4)

include/dftd_ncoord.h:42

[nitpick] The overload that takes explicit cn, dcndr, and cutoff is no longer used in implementation; consider removing this overload to simplify the public API.

    int get_ncoord( // with ghost atoms

include/dftd_ncoord.h:32

[nitpick] Public fields and multiple overloads merit a high‐level doc comment explaining intended usage, ownership of cn/dcndr, and behavior of the virtual methods.

class NCoordBase

test/test_ncoord.cpp:64

[nitpick] Tests currently only cover the default counting parameters; consider adding cases with non‐default kcn and norm_exp to verify parameterization behavior.

  NCoordErf ncoord_erf(7.5, 1.0, 9999.9);

include/qceeq_param.h:18

[nitpick] Defining large static arrays in a header can lead to ODR issues and long compile times; consider marking them inline constexpr or moving the definitions to a .cpp.

static const double xi[MAXELEMENT]{

src/dftd_ncoord.cpp

Copilot · 2025-06-16T11:49:28Z

src/dftd_ncoord.cpp

@@ -312,12 +320,10 @@ int get_ncoord_erf(
  return EXIT_SUCCESS;
 };

-int ncoord_erf(
+int NCoordBase::ncoord_base(


[nitpick] The loops in ncoord_base, dr_ncoord_base, ncoord_d4, and dncoord_d4 are nearly identical; consider refactoring common logic into a templated or higher‐order function to reduce duplication.

This rises the question why we use dcndr(3*N, N) for dncoord_d4 and dcndr(N, 3*N) for ncoord_base. We can open an issue if this should be changed, but I don't think this should be done in this PR.

Yeah, this should all be N, 3N.

As mentioned below, this is a lot of work and should be addressed in a different PR. Also this could have major conflicts with my EEQ-BC implementation. I would suggest comming back to this after the EEQ-BC implementation.

marvinfriede · 2025-06-16T11:53:28Z

include/qceeq_param.h

@@ -0,0 +1,110 @@
+/*


This file should not be here.

Thanks for catching this.

marvinfriede · 2025-06-16T11:54:24Z

include/dftd_ncoord.h

+      const TIVector&,
+      const TMatrix<double>&);
+    // Calculate the derivative of the coordination number
+    int dr_ncoord_base(


Suggested change

int dr_ncoord_base(

int dncoord_base(

Just for a more consistent naming.

I find d<name> not ideal because it does not give insight on the actual derivative like dncoord_dr or dcndr do. However, I realize that some functions provide multiple derivatives so lets stick with the used convention.

marvinfriede · 2025-06-16T12:00:23Z

include/dftd_ncoord.h

+    // erf() based counting function
+    double count_fct(double) const override;
+    // derivative of the erf() based counting function
+    double dr_count_fct(double) const override;


Suggested change

double dr_count_fct(double) const override;

double dcount_fct(double) const override;

marvinfriede · 2025-06-16T12:07:50Z

src/dftd_dispersion.cpp

    );
    if (info != EXIT_SUCCESS) return info;

-    cn.Delete();
+    ncoord_erf.cn.Delete();


I think the destructor should also delete these arrays.

marvinfriede · 2025-06-16T12:13:15Z

include/dftd_ncoord.h

+      const TMolecule&,
+      const TIVector&,
+      const TMatrix<double>&);
+    // Get the DFT-D4 coordination number


The D4 CN should not be part of the base class. In the Fortran code, this is also a separate subclass (of an error function CN class).

I think the best way would be to include the den factor in the countf:

cpp-d4/src/dftd_ncoord.cpp

Lines 460 to 461 in d61715f

den = k4 * exp(-pow((fabs(en[izp] - en[jzp]) + k5), 2) / k6);

countf = den * erf_count(kn, rr);

Then we could create a new derived type of the NCoordBase e.g. NCoordErfD4 that uses ncoord_base. This way we could delete all the "doublicate" _d4 functions and would implement consistent dimensions for dcndr(N, 3N).

I just realized that changing dcndr(3N, N) to dcndr(N, 3N) is a lot more work than I thought and should be a PR for itself. Since I can not merge the _d4 functions and into the base class functions without this I will not implement this for now.

Thomas3R · 2025-06-18T13:13:03Z

test/test_grad.cpp

@@ -126,6 +126,7 @@ int test_pbed4_mb01() {
  par.s8 = 0.95948085;
  par.a1 = 0.38574991;
  par.a2 = 4.80688534;
+  par.s10 = 0.0;


Valgrind was bothering me about par.s10 being uninitialized.

Thomas3R added 3 commits June 13, 2025 11:11

refactor: move charge models to new multicharge namespace and create …

5f035a2

…ChargeModel class

refactor: create class for coordination number calculation

7fec355

Update comment in dftd_ncoord.cpp

b85b2c7

Thomas3R force-pushed the ncoord branch from 4d4029a to b85b2c7 Compare June 13, 2025 09:19

marvinfriede requested a review from Copilot June 16, 2025 11:45

Copilot AI reviewed Jun 16, 2025

View reviewed changes

marvinfriede reviewed Jun 16, 2025

View reviewed changes

Thomas3R added 4 commits June 18, 2025 10:42

Move cn and dcndr allocation into ncoord and dncoord functions.

6886aa7

NCoordBase constructor initializer list and code cleanup

c65dc27

rename functions for consistency

b2a6fc0

Leave deletion of arrays to object destructor

fc8baed

Thomas3R commented Jun 18, 2025

View reviewed changes

	double dr_count_fct(double) const override;
	double dcount_fct(double) const override;

	den = k4 * exp(-pow((fabs(en[izp] - en[jzp]) + k5), 2) / k6);
	countf = den * erf_count(kn, rr);

refactoring: Create class for coordination number calculation #43

Are you sure you want to change the base?

refactoring: Create class for coordination number calculation #43

Uh oh!

Conversation

Thomas3R commented Jun 12, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Jun 16, 2025

Choose a reason for hiding this comment

Uh oh!

Thomas3R Jun 18, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Thomas3R Jun 18, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

marvinfriede Jun 16, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Thomas3R Jun 18, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Thomas3R Jun 18, 2025 •

edited

Loading

Thomas3R Jun 18, 2025 •

edited

Loading

marvinfriede Jun 16, 2025 •

edited

Loading

Thomas3R Jun 18, 2025 •

edited

Loading