Skip to content

EmpiricalDispersionModel#332

Merged
EBB2675 merged 2 commits intodevelopfrom
semantic-cleanup
Feb 26, 2026
Merged

EmpiricalDispersionModel#332
EBB2675 merged 2 commits intodevelopfrom
semantic-cleanup

Conversation

@EBB2675
Copy link
Collaborator

@EBB2675 EBB2675 commented Feb 26, 2026

Purpose

This PR refines the dispersion schema to cover empirical dispersion models only and removes non-empirical/nonlocal correlation entries from this path.

After some parsing and discussions with Denis, we realized that EmpiricalDispersionModel would be a better fit here

Scope

Included

  • Renamed the dispersion contribution class to EmpiricalDispersionModel for consistency with existing semantics.
  • Restricted supported dispersion model types to empirical families (e.g. D2/D3/D3BJ/D4/TS/TS-SCS/MBD/MBD@rsSCS/XDM).
  • Removed nonlocal-correlation-specific options from this branch of the schema.
  • Updated related dispersion numerical settings and tests to match the empirical-only scope.

Out of scope

Context & Links

Reminder: Link related issues or PRs (in the sidebar or below). Add short description for any needed context.

Reviewer Notes

I renamed the related numerical settings to EmpiricalDispersionKnob and EmpiricalDispersionSettings to match EmpiricalDispersionModel and to keep this branch empirical-only. Do you prefer these explicit names, or shorter DispersionKnob/DispersionSettings with the empirical scope implied by context?

Status

  • Draft / In progress
  • Ready for review
  • Blocked (explain):

Breaking Changes

  • (explain):
    It renames sections, but these have not yet been used in any of the merged parsers

Dependencies / Blockers

  • None
  • Open design questions (explain):
  • External dependencies (explain):

Testing / Validation

How was this change validated?

  • None (explain):
  • Unit tests
  • Integration tests
  • Manual testing (explain):

Copy link
Contributor

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 narrows the dispersion-related schema to empirical dispersion corrections only by renaming the contribution section to EmpiricalDispersionModel and removing nonlocal-correlation-specific entries from this schema path (to be handled separately via NonlocalCorrelation).

Changes:

  • Renamed ExplicitDispersionModel to EmpiricalDispersionModel and restricted the allowed model enum to empirical families only.
  • Removed nonlocal-correlation-related fields/options (e.g., VV10/rVV10 families, nonlocal_kernel, is_embedded_in_xc) from the empirical dispersion contribution and numerical knob schema.
  • Updated unit tests to use EmpiricalDispersionModel.

Reviewed changes

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

File Description
tests/test_model_method.py Updates imports/usage and assertions to the renamed EmpiricalDispersionModel.
src/nomad_simulations/schema_packages/numerical_settings.py Refines dispersion knob/settings semantics to empirical-only (removes nonlocal-kernel-related enum values/docs).
src/nomad_simulations/schema_packages/model_method.py Renames the dispersion contribution section and removes nonlocal-correlation variants/options from the empirical dispersion model enum/schema.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Collaborator

@Bernadette-Mohr Bernadette-Mohr left a comment

Choose a reason for hiding this comment

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

Not my domain exactly, but looks fine to me!

Copy link
Collaborator

@ndaelman-hu ndaelman-hu left a comment

Choose a reason for hiding this comment

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

Update to section names and definition terminology. Passes the testing. All good.

@EBB2675 EBB2675 merged commit 02a31ab into develop Feb 26, 2026
6 checks passed
@EBB2675 EBB2675 deleted the semantic-cleanup branch February 26, 2026 10:03
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.

5 participants