Conversation
Pull Request Test Coverage Report for Build 22352739452Details
💛 - Coveralls |
There was a problem hiding this comment.
Pull request overview
This PR introduces a dedicated NonlocalCorrelation schema section to represent nonlocal correlation functionals (VV10/rVV10, vdW-DF families) and integrates it into DFT normalization. The implementation automatically parses and strips nonlocal addons (e.g., +VV10, +rVV10) from XC functional keys, creates or reuses corresponding NonlocalCorrelation contributions, and establishes typed references between contributions and their paired XC functionals.
Changes:
- Added
NonlocalCorrelationclass with typedxc_partner_reffor linking to base XC functional - Implemented
split_xc_and_addonsutility to parse nonlocal addons from functional keys - Modified
DFT.normalizeto handle addon parsing, contribution creation/reuse, and partner reference linking - Replaced string-based
xc_partnerwith typedxc_partner_refinExplicitDispersionModel - Removed obsolete fields (
is_embedded_in_xc,nonlocal_kernel) and enum values fromExplicitDispersionModel
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
src/nomad_simulations/schema_packages/utils/xc_addons.py |
New utility module for parsing nonlocal correlation addons from XC keys |
src/nomad_simulations/schema_packages/model_method.py |
Added NonlocalCorrelation class, updated ExplicitDispersionModel, modified DFT.normalize for addon handling |
tests/test_model_method.py |
Added tests for addon parsing, contribution creation/reuse, and partner reference linking |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| existing_nonlocal = None | ||
| for contribution in self.contributions or []: | ||
| if not isinstance(contribution, NonlocalCorrelation): | ||
| continue | ||
| if contribution.type in (None, nonlocal_corr_addon): | ||
| existing_nonlocal = contribution | ||
| break | ||
|
|
||
| if existing_nonlocal is None: |
There was a problem hiding this comment.
The reuse logic creates a new NonlocalCorrelation contribution when an existing one has a different type (e.g., existing type='VV10' but addon='rVV10'). This results in multiple nonlocal correlation contributions with different types, which may indicate a parser error or conflict between explicitly created contributions and the functional_key. Consider logging a warning when this occurs to help identify configuration issues.
| existing_nonlocal = None | |
| for contribution in self.contributions or []: | |
| if not isinstance(contribution, NonlocalCorrelation): | |
| continue | |
| if contribution.type in (None, nonlocal_corr_addon): | |
| existing_nonlocal = contribution | |
| break | |
| if existing_nonlocal is None: | |
| existing_nonlocal = None | |
| conflicting_nonlocal_types = set() | |
| for contribution in self.contributions or []: | |
| if not isinstance(contribution, NonlocalCorrelation): | |
| continue | |
| if contribution.type in (None, nonlocal_corr_addon): | |
| existing_nonlocal = contribution | |
| break | |
| if contribution.type is not None: | |
| conflicting_nonlocal_types.add(contribution.type) | |
| if existing_nonlocal is None: | |
| if conflicting_nonlocal_types: | |
| logger.warning( | |
| "DFT.xc nonlocal correlation type conflict detected; " | |
| "creating additional NonlocalCorrelation contribution.", | |
| existing_nonlocal_types=sorted(conflicting_nonlocal_types), | |
| requested_nonlocal_type=nonlocal_corr_addon, | |
| functional_key=getattr(self.xc, "functional_key", None), | |
| ) |
There was a problem hiding this comment.
thx for the suggestion, i will wait for nathan's comment
| _NONLOCAL_SUFFIX_RE = re.compile( | ||
| r'^(?P<base>.+?)\s*\+\s*(?P<addon>RVV10|VV10)\s*$', | ||
| re.IGNORECASE, | ||
| ) |
There was a problem hiding this comment.
The regex pattern uses .+? for the base functional which requires at least one character, but it also strips the result. This means inputs like "+ VV10" (with leading +) will not match. While this is likely intentional, consider documenting this behavior or adding validation to handle malformed inputs more explicitly.
| def split_xc_and_addons(raw: str) -> tuple[str | None, str | None, str | None]: | ||
| """ | ||
| Split an XC key into baseline XC and optional add-ons. | ||
|
|
||
| Returns: | ||
| (base_xc_key, explicit_dispersion_suffix, nonlocal_corr_addon) |
There was a problem hiding this comment.
The function signature indicates it should return (base_xc_key, explicit_dispersion_suffix, nonlocal_corr_addon), but the explicit_dispersion_suffix is always None in the implementation. This suggests the function is designed for future expansion to handle explicit dispersion addons like '+D3', '+D4', etc., but this is not currently implemented. Consider either implementing this functionality or removing the unused return value to avoid confusion.
| _NONLOCAL_SUFFIX_RE = re.compile( | ||
| r'^(?P<base>.+?)\s*\+\s*(?P<addon>RVV10|VV10)\s*$', | ||
| re.IGNORECASE, | ||
| ) |
There was a problem hiding this comment.
The regex pattern only captures 'VV10' and 'RVV10' (case-insensitive). However, the NonlocalCorrelation.type enum includes many more variants: 'vdW-DF', 'vdW-DF2', 'vdW-DF-cx', 'optB88-vdW', 'optB86b-vdW'. If these are meant to be parsed from functional keys like 'PBE+vdW-DF', the regex needs to be extended. If they are only set explicitly by parsers, this should be documented in the function's docstring.
| def split_xc_and_addons(raw: str) -> tuple[str | None, str | None, str | None]: | ||
| """ | ||
| Split an XC key into baseline XC and optional add-ons. | ||
|
|
||
| Returns: | ||
| (base_xc_key, explicit_dispersion_suffix, nonlocal_corr_addon) | ||
|
|
||
| Notes: | ||
| - For now, only nonlocal-correlation add-ons are recognized: +VV10 / +rVV10. | ||
| - Unknown forms are left untouched as the base XC key. | ||
| """ | ||
| if raw is None: | ||
| return None, None, None | ||
|
|
||
| text = raw.strip() | ||
| if not text: | ||
| return None, None, None | ||
|
|
||
| match = _NONLOCAL_SUFFIX_RE.match(text) | ||
| if not match: | ||
| return text, None, None | ||
|
|
||
| base_xc_key = match.group('base').strip() or None | ||
| addon_raw = match.group('addon').upper() | ||
| nonlocal_corr_addon = 'rVV10' if addon_raw == 'RVV10' else 'VV10' | ||
|
|
||
| return base_xc_key, None, nonlocal_corr_addon |
There was a problem hiding this comment.
The function returns (base_xc_key, None, nonlocal_corr_addon) for valid nonlocal addons, or (text, None, None) for inputs without recognized addons. However, there's no validation or logging for malformed inputs like "PBE++VV10" (double plus), "VV10+PBE" (reversed order), or "PBE+VV10+rVV10" (multiple addons). Consider adding validation or documenting expected input format constraints.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def split_xc_and_addons(raw: str) -> tuple[str | None, str | None, str | None]: | ||
| """ | ||
| Split an XC key into baseline XC and optional add-ons. | ||
|
|
||
| Returns: | ||
| (base_xc_key, explicit_dispersion_suffix, nonlocal_corr_addon) | ||
|
|
||
| Notes: | ||
| - For now, only nonlocal-correlation add-ons are recognized: +VV10 / +rVV10. | ||
| - Unknown forms are left untouched as the base XC key. | ||
| """ | ||
| if raw is None: | ||
| return None, None, None |
There was a problem hiding this comment.
split_xc_and_addons is annotated as raw: str, but the implementation explicitly handles raw is None and callers (e.g. DFT.normalize) can pass XCFunctional.functional_key which may be None. This will trip mypy; please change the signature to accept str | None (and adjust the return annotation/docstring accordingly).
| type=str, | ||
| xc_partner_ref = Quantity( | ||
| type=Reference(SectionProxy('XCFunctional')), | ||
| description="Base XC functional used/tuned for (e.g. 'PBE', 'SCAN', 'B3LYP').", |
There was a problem hiding this comment.
xc_partner_ref is now a Reference(XCFunctional), but the description still reads like this is a string value (e.g. 'PBE', 'SCAN'). Please update the description to clearly state it stores a reference to the partner XCFunctional section (and, if needed, where to find its name/key).
| description="Base XC functional used/tuned for (e.g. 'PBE', 'SCAN', 'B3LYP').", | |
| description=( | |
| "Reference to the partner XCFunctional section used or tuned for this " | |
| "dispersion model. The corresponding functional label (e.g. 'PBE', " | |
| "'SCAN', 'B3LYP') is stored in the referenced XCFunctional entry." | |
| ), |
|
This PR (among other things) semantizes
Good questions — I did some (admittedly cursory) research into how codes report these functionals. I may well be missing cases, so take this with a grain of salt. From what I could find, no more than one or two programs or libraries actually attach a nonlocal correlation tag to the functional name. It's more common in the literature, but most codes — VASP, QE, SIESTA, ABINIT — set it through separate input fields or flag combinations. So in practice it may be better to just have the parser set I understand the impulse to automate this and make life easier for parser developers, but the automation may be somewhat premature here. That said, I don't have a strong opinion on this question by itself — it's the second one that gives me a clearer preference.
I'm not entirely sure what you mean by creating multiple entries (more on that below), but it sounds like it could be a deeper-lying bug that would take significant time to trace. It may just be easier to remove the normalization logic and defer to the parser — in that case there should only ever be one We could of course retain the inference if it's much more commonly needed in the QC community than my quick search suggests. But then we'd need to investigate the duplication or design cleanup logic to merge entries, which is more involved. Question to @EBB2675With "a second entry" do you mean two separate archives or two subsections within one? And did you notice this when running a parsing example? I ask because the behavior sounds in line with issues I've seen with the mapping annotations parser — I could see that being the underlying cause. |
that sounds nicer, i will remove these normalizations and re-tag you for final look 👍 |
Purpose
This PR introduces a dedicated
NonlocalCorrelationschema section and integrates it into DFT normalization for nonlocal-correlation add-ons (+VV10/+rVV10). It also standardizes XC partner linking through typed references.Scope
Included
1. Added
NonlocalCorrelationclassVV10,rVV10,vdW-DF,vdW-DF2,vdW-DF-cx,optB88-vdW, andoptB86b-vdW.type: normalized nonlocal-correlation identityxc_partner_ref: reference to the pairedXCFunctional2. Integrated nonlocal add-on handling into
DFT.normalizexc.functional_keyfor trailing+VV10/+rVV10.SCAN + rVV10->SCAN).NonlocalCorrelationcontribution when possible.NonlocalCorrelation(type=...)contribution when needed.NonlocalCorrelation.xc_partner_refto the activedft.xc.3. Standardized XC partner linkage for empirical dispersion
EmpiricalDispersionModelnow usesxc_partner_refas well.DFT.normalizesetsEmpiricalDispersionModel.xc_partner_ref(when missing) to the activedft.xc.4. Removed legacy partner string field
xc_partnerstring-based partner field/usage.xc_partner_ref).Why no libvdwxc canonicalization layer in this PR
I could not add a
libvdwxccanonical-label mapping because, unlike LibXC, there is no stable and broadly adopted canonical label/ID registry equivalent to LibXC’s taxonomy. This PR therefore keeps nonlocal normalization schema-level (NonlocalCorrelation.type) and parser-driven.Context & Links
xc.functional_key == 'SCAN'one
NonlocalCorrelation(type='rVV10')partner linkage to the base XC via
xc_partner_refReviewer Notes
We currently infer nonlocal correlation only from functional_key patterns +VV10 / +rVV10 (via
split_xc_and_addons()), butNonlocalCorrelation.typealso includes vdW-DF variants (vdW-DF, vdW-DF2, vdW-DF-cx, optB88-vdW, optB86b-vdW). Should we (a) extend inference to recognize these variants infunctional_key, (b) restrict the enum to what we infer automatically (VV10/rVV10) and expect vdW-DF variants to be set explicitly by parsers, or (c) keep both but document that only VV10/rVV10 are inferred today? @ndaelman-huIf an existing
NonlocalCorrelationcontribution conflicts with what we infer fromfunctional_key, we currently create a second entry, which is not ideal. What would you suggest? @ndaelman-huStatus
Breaking Changes
Dependencies / Blockers
Testing / Validation
How was this change validated?
Unit tests
Integration tests
Added/updated tests in
tests/test_model_method.pyto cover:creation of
NonlocalCorrelationfromSCAN + rVV10reuse of existing matching
NonlocalCorrelation(type='VV10')filling missing
NonlocalCorrelation.typelinking
NonlocalCorrelation.xc_partner_reftodft.xclinking
EmpiricalDispersionModel.xc_partner_reftodft.xc