Skip to content
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

Move coordination number and common data to mctc-lib #135

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

thfroitzheim
Copy link
Contributor

PR to move the coordination number to the mctc-lib (see PR #71 there). This avoids duplicated code in all our projects and makes the addition of new coordination numbers simpler. The full functionality and all tests were moved to mctc-lib. Additionally, I moved the pairwise radii to mctc-lib as common data used in future projects at different places.

This PR starts as a draft pointing to the mctc-lib branch until it is merged.

Copy link

codecov bot commented Mar 17, 2025

Codecov Report

Attention: Patch coverage is 33.33333% with 10 lines in your changes missing coverage. Please review.

Project coverage is 66.75%. Comparing base (395e2ff) to head (1fb787a).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/dftd3/ncoord.f90 33.33% 4 Missing and 6 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #135      +/-   ##
==========================================
- Coverage   67.07%   66.75%   -0.32%     
==========================================
  Files          34       33       -1     
  Lines        4687     4666      -21     
  Branches     1647     1646       -1     
==========================================
- Hits         3144     3115      -29     
- Misses        632      636       +4     
- Partials      911      915       +4     

☔ View full report in Codecov by Sentry.
📢 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.

Copy link
Member

@awvwgk awvwgk left a comment

Choose a reason for hiding this comment

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

Can we reimplement the removed modules based on the new functionality in mctc_ncoord? This would ensure that we don't remove API features in a minor release.

@thfroitzheim
Copy link
Contributor Author

Yes, I can do that. I would use these modules then throughout the code. Still, we should remove it with a later 2.0.0 release (I will open an issue after this is done).

@thfroitzheim thfroitzheim marked this pull request as ready for review March 29, 2025 13:23
@thfroitzheim thfroitzheim requested a review from awvwgk March 31, 2025 09:33
Copy link
Member

@awvwgk awvwgk 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 now.

Let's merge this once we have a new mctc-lib release with the full and final API, such that we don't need to depend on the latest git version.

@thfroitzheim
Copy link
Contributor Author

We can do that. The only remaining update that has to be reviewed before is in tblite, here.

In the long term, we (@LSeidler and I) also think about pulling the periodic features (lattice vectors and Wigner-Seitz cell) out of the projects and into mctc-lib. But this shouldn't be a problem, since we can keep the API unchanged.

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