Skip to content

refactoring: Create class for coordination number calculation #43

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 14 commits into from
Aug 13, 2025

Conversation

Thomas3R
Copy link
Contributor

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.

Copilot

This comment was marked as outdated.

Copy link
Member

@marvinfriede marvinfriede left a comment

Choose a reason for hiding this comment

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

The issue with different matrix orderings should be fixed first (to N, 3N). Otherwise, the object-oriented implementation does not make much sense, because we are duplicating the coordination number calculation again.

Thomas3R added 2 commits July 18, 2025 11:35
- Merged ncoord_d4 and ncoord_base
- Fixed dcndr calculation
@marvinfriede marvinfriede requested a review from Copilot August 13, 2025 07:18
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors coordination number calculation functionality by introducing a new class hierarchy with NCoordBase as the base class. The design enables customization of counting functions and parameters for different coordination number calculation methods while centralizing common functionality.

  • Creates NCoordBase class with virtual methods for counting functions and electronegativity factors
  • Implements NCoordErf and NCoordErfD4 derived classes with specific counting behavior
  • Moves cn and dcndr vectors to be member variables of the base class

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.

Show a summary per file
File Description
include/dftd_ncoord.h Defines new class hierarchy with base and derived classes for coordination number calculations
src/dftd_ncoord.cpp Implements the new class methods and refactors existing functions to use class-based approach
src/dftd_eeq.cpp Updates EEQ charge calculation to use new NCoordErf class
src/dftd_dispersion.cpp Updates dispersion calculation to use new NCoordErfD4 class
test/test_ncoord.cpp Adds comprehensive numerical gradient tests for both coordination number classes
test/test_grad.cpp Adds numerical gradient test for charge derivatives and fixes memory management
test/test_ghost.cpp Updates ghost atom tests to use new class interface
include/dftd_cblas.h Fixes matrix-vector multiplication logic for transposed operations

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Co-authored-by: Marvin Friede <[email protected]>
@marvinfriede marvinfriede merged commit 2a4b3a4 into dftd4:main Aug 13, 2025
17 checks passed
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.

2 participants