-
Notifications
You must be signed in to change notification settings - Fork 2
Quantum chemistry add-ons to Density Functional Theory #237
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
Conversation
Pull Request Test Coverage Report for Build 19224631192Details
💛 - Coveralls |
|
What do you mean exactly with Point 2? |
258b00e to
fea8c63
Compare
|
@ladinesa @ndaelman-hu @JFRudzinski please take a look at LibXC canonicalization and the new DFT architecture I added As you can understand, there are multiple ways this canonicalization can take place. this is one complete implementation. I do not have any personal preferences. I am ready to adopt to whatever you suggest 👍 |
For me this looks very nice, but I have no insight into the domain details here. I expect @ndaelman-hu will give you more specific feedback/confirmation |
ndaelman-hu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like the DFT mapping additions. A few quantities that need to be added.
Also comments on the structure, suggesting some reorganization. I think that if these are addressed, you can go ahead and merge. No need for another review.
Disclaimer: didn't check the actual code implementations too much (would first need to understand the exact workflow). You could ask the copilot reviewer.
tests/utils/test_libxc.py
Outdated
| _COMMON_XC_CASES = [ | ||
| # ---------------- LDA ---------------- | ||
| ('LDA', 'LDA', ['XC_LDA_X', 'XC_LDA_C_PW', 'XC_LDA_C_PZ']), | ||
| ('PW92', 'LDA', ['XC_LDA_C_PW']), | ||
| ('PZ81', 'LDA', ['XC_LDA_C_PZ']), | ||
| ('VWN', 'LDA', ['XC_LDA_C_VWN']), | ||
| ('SVWN', 'LDA', ['XC_LDA_X', 'XC_LDA_C_VWN']), | ||
| ('LSDA', 'LDA', ['XC_LDA_X', 'XC_LDA_C_VWN']), | ||
| ('LDA+PW', 'LDA', ['XC_LDA_X', 'XC_LDA_C_PW']), | ||
| # ---------------- GGA ---------------- | ||
| ('PBE', 'GGA', ['XC_GGA_X_PBE', 'XC_GGA_C_PBE']), | ||
| ('PBEsol', 'GGA', ['XC_GGA_X_PBE_SOL', 'XC_GGA_C_PBE_SOL']), | ||
| ('revPBE', 'GGA', ['XC_GGA_X_RPBE', 'XC_GGA_C_PBE']), | ||
| ('RPBE', 'GGA', ['XC_GGA_X_RPBE', 'XC_GGA_C_PBE']), | ||
| ('PW91', 'GGA', ['XC_GGA_X_PW91', 'XC_GGA_C_PW91']), | ||
| ('BLYP', 'GGA', ['XC_GGA_X_B88', 'XC_GGA_C_LYP']), | ||
| ('BP86', 'GGA', ['XC_GGA_X_B88', 'XC_GGA_C_P86']), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please import this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these are also present in expand.py, correct? Unfortunately, there isn't much of a way for testing these kinds of mappings [1]. You basically generated them twice and check for typos.
Personally, I feel that you can safely remove this test.
[1]: I guess that with a very granular format specification, you could run that to impose compliance. But I think that's overkill in this case.
|
@EBB2675 , at this stage, I'd also request feedback from the co-pilot reviewer. |
There was a problem hiding this 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 adds quantum chemistry extensions to Density Functional Theory (DFT) capabilities by introducing LibXC-based XC functional normalization and additional quantum chemistry model methods. The changes include a comprehensive XC functional expansion system, new model method classes for implicit solvation, dispersion corrections, and relativistic treatments, along with extensive test coverage.
Key Changes
- Implements LibXC-based XC functional canonicalization with registry lookup and expansion capabilities
- Adds new model method classes: ImplicitSolvationModel, ExplicitDispersionModel, RelativityModel, and CoreElectronTreatment
- Refactors DFT class to use new XCFunctional/XCComponent schema with automatic Jacob's ladder derivation
Reviewed Changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/utils/test_libxc.py | Comprehensive test suite for LibXC functional expansion and DFT normalization |
| tests/test_model_method.py | Extended tests for DFT class with XC component handling and parameter derivation |
| src/nomad_simulations/schema_packages/utils/libxc/registry.py | LibXC registry lookup functionality with label and ID-based queries |
| src/nomad_simulations/schema_packages/utils/libxc/expand.py | XC functional expansion logic with alias tables and fallback mechanisms |
| src/nomad_simulations/schema_packages/utils/libxc/build.py | XCComponent factory functions for creating components from LibXC data |
| src/nomad_simulations/schema_packages/utils/libxc/init.py | Module interface exposing build functions |
| src/nomad_simulations/schema_packages/utils/libxc/README.md | Documentation for LibXC canonicalization pipeline and supported formats |
| src/nomad_simulations/schema_packages/numerical_settings.py | Added FrozenCore, IntegralDecomposition, and OrbitalLocalization settings classes |
| src/nomad_simulations/schema_packages/model_method.py | Major refactor with new quantum chemistry model classes and updated DFT implementation |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
ndaelman-hu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some structural remarks in the schema. These should be addressed.
Normally, I'd give this "Comment", but since no further iteration is needed and you typically follow up well with the comments, I'll add it as "Approve". Note that some older comments are also still open.
| except KeyError: | ||
| return 'unavailable' | ||
| return self._jacobs_ladder_map.get(highest_rung_abbrev, 'unavailable') | ||
| # ? Do we need to define `type` for DFT+U? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
93ceaf2 to
fbfe06e
Compare
|
Hey, @EBB2675, I will check this later next week. It looks good, but there are a couple of sections I will move out of ModelMethodElectronic, as these are only relevant for quantum chemistry simulations. Furthermore, I am not sure about the role here of dielectrics and so. In cond mat, these are typically something else (I am guessing, I dont know. I would like to check this, or if you know, please let me know :-) ). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated 1 comment.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
kubanmar
left a comment
There was a problem hiding this 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.
JosePizarro3
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good, just some minor thing about a couple of sub-sections in ModelMethodElectronic that I would move out.
|
Hi @ladinesa @JosePizarro3 @JFRudzinski, @Bernadette-Mohr , @ndaelman-hu , @kubanmar , This PR is ready for rigorous DFT parser testing.
If you prefer a different name for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, great work on this PR! Structurally it looks good overall.
Given how mature it is already, I just went over all my old comments. I noticed that I missed some of your replies under all the e-mail notifications.
I followed up on a few comments. I just have a few suggestions for minor tweaks.
I also opened a bunch of issues where you suggested. Feel free to add to them.
Edit: I added a bunch of our comments via VS Code's GitLens, but they appear as new messages now without their thread... My apologies for the mess.
| """, | ||
| ) | ||
|
|
||
| approximation = Quantity( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment has not yet been applied. It's not a big deal, but would mean 1 field less when parsing.
ac07fd5 to
0dd9b0b
Compare
0dd9b0b to
e0e8c57
Compare
No description provided.