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

Moving the coordination number to mctc-lib #71

Draft
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

thfroitzheim
Copy link
Contributor

It would be quite beneficial to have a centralized place to calculate the CN since it appears in all of our projects (xtb, tblite, dftd4, dftd3, multicharge, DRACO, ...). Moreover, there are now several different CNs based on slight variations of the counting function, which must be integrated in several places.

mctc-lib might be the right spot since it is purely based on the structure, but I am open to alternatives (@awvwgk, @Albkat, @marvinfriede). As an example, I moved the tblite implementation to the mctc-lib. This also includes the cutoffs for periodicity and part of the data sections (maybe we should move the whole as literature radii and EN are also frequently used).

If we should do this, I will still clean up a few things in the CN (i.e. rename the gfn CN to dexp for double exponential) to make it general.

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.

The update for the GH actions can be a separate PR.

@thfroitzheim
Copy link
Contributor Author

Sure. I moved the update of the GH actions to (#72).

I am mostly interested if you think that mctc-lib is the right place for the coordination numbers.

@marvinfriede
Copy link
Member

I agree with moving the CNs and data here. I also did that for the autodiff versions (CN, data).

Copy link

codecov bot commented Nov 11, 2024

Codecov Report

Attention: Patch coverage is 56.81159% with 447 lines in your changes missing coverage. Please review.

Project coverage is 68.48%. Comparing base (77f65c6) to head (cd64b3d).
Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
test/test_ncoord.f90 58.15% 24 Missing and 253 partials ⚠️
src/mctc/cutoff.f90 39.06% 26 Missing and 13 partials ⚠️
src/mctc/ncoord/type.f90 77.55% 9 Missing and 13 partials ⚠️
src/mctc/data/vdwrad.f90 0.00% 20 Missing ⚠️
src/mctc/ncoord.f90 35.71% 0 Missing and 18 partials ⚠️
src/mctc/ncoord/erf/dftd4.f90 48.14% 5 Missing and 9 partials ⚠️
src/mctc/ncoord/erf/en.f90 55.55% 4 Missing and 8 partials ⚠️
src/mctc/ncoord/exp.f90 57.69% 4 Missing and 7 partials ⚠️
src/mctc/ncoord/dexp.f90 68.96% 3 Missing and 6 partials ⚠️
src/mctc/data/atomicrad.f90 0.00% 8 Missing ⚠️
... and 3 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #71      +/-   ##
==========================================
- Coverage   69.79%   68.48%   -1.32%     
==========================================
  Files          64       77      +13     
  Lines        8618     9674    +1056     
  Branches     2579     3045     +466     
==========================================
+ Hits         6015     6625     +610     
- Misses        782      885     +103     
- Partials     1821     2164     +343     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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.

3 participants