add AtomsState.partial_charge for atom-centered fractional charges#335
add AtomsState.partial_charge for atom-centered fractional charges#335
AtomsState.partial_charge for atom-centered fractional charges#335Conversation
AtomsState.partial_chargeAtomsState.partial_charge for atom-centered fractional charges
There was a problem hiding this comment.
Pull request overview
This PR adds a new partial_charge field to the AtomsState class to support atom-centered fractional charges from force fields and population analyses (e.g., Mulliken, Hirshfeld, CM5, NPA, RESP). This field is distinct from the existing formal integer charge field which represents oxidation states.
Changes:
- Added
partial_chargeQuantity toAtomsStatewithnp.float64type andelementary_chargeunit - Added unit test verifying that
partial_chargecan coexist with and is independent of the formalchargefield
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/nomad_simulations/schema_packages/atoms_state.py | Adds new partial_charge field to store fractional atomic charges from population analyses and force fields |
| tests/test_atoms_state.py | Adds test to verify partial_charge and formal charge can coexist independently |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Atom-centered partial charge used in force fields or population analyses | ||
| (e.g., Mulliken, Hirshfeld, CM5, NPA, RESP). This quantity is distinct from | ||
| the formal integer oxidation-like `charge`. | ||
| """, |
There was a problem hiding this comment.
I like your definition better than mine, great!
ndaelman-hu
left a comment
There was a problem hiding this comment.
Clean separation of different concepts for charge.
There was a problem hiding this comment.
Thank you for the dedication to testing. Since we do not have any automated systems to populate one charge from the other, this test could also be safely removed. This marginally reduces the test stack. Conversely, if you want to safeguard against future normalization improperly deriving one from the other, you can leave it in.
There was a problem hiding this comment.
Conversely, if you want to safeguard against future normalization improperly deriving one from the other, you can leave it in.
Thanks alot, that makes sense. I think i will indeed keep this one test as a lightweight regression guard for the future 👍
Purpose
This PR adds a dedicated
partial_chargefield toAtomsStateso parsers can store atom-centered fractional charges without overloading the existing formal integercharge.Scope
Included
AtomsState.chargeis intentionally formal/integer and defines the elemental charge. For MD and population-related runs, we also need a place for fractional per-atom charges (e.g., Mulliken, Hirshfeld, CM5, NPA, RESP).Context & Links
Reviewer Notes
Anything reviewers should pay special attention to? Trade-offs, known limitations, or areas of uncertainty.
Status
Breaking Changes
Dependencies / Blockers
Testing / Validation
How was this change validated?