Conversation
- Simplify `is_derived` - Clean dependencies
- Mark unused or superfluous quantities
- Update some TODOs
- Add `sub_plots`
- Extend normalization to: -- allow for normalization only once -- move up normalization of the parent class
- Change `sub_plots` to use pre-existing sub_plot figures or produce them - Apply contribution structure to properties like Energy, Force, and Eigenvalues -- Correct mypy error in `order_eigenvalues` by simplifying return type to just tuples - Prune superfluous initializers
- Add naming logic to `Variables` base section
- Remove superfluous test checking the no. subsections in outputs
JFRudzinski
left a comment
JFRudzinski
left a comment
There was a problem hiding this comment.
Thanks for your work on this @ndaelman-hu !
I think most of it is good as is.
- My comments referring to the TODOs could be addressed simply by coming to a consensus and adding to the TODO
- My main concern is the change in contributions implementation, I don't think it is functioning now in the way we intended. Let's discuss and either adjust it or move it to another PR
| is populated, the quantity `is_derived` is set to True via normalization. | ||
| """, | ||
| ) | ||
| ) # TODO: only used for electronic states, remove |
There was a problem hiding this comment.
I agree that this doesn't belong under PP. Are there real use cases of properties referencing individual atoms such that we need to generalize Outputs.modelsystem_ref?
I think if there is a per-atom PP, then Output just references the ModelSystem with the corresponding list of atoms in particles_indices
There was a problem hiding this comment.
I have some use cases of properties referencing individual atoms, and referencing through particle_indices sounds suitable from my end 👍
There was a problem hiding this comment.
It might be pointing to specific orbital states too.
There are indeed properties which are per atom or per orbital, while there might be a total one as well. Either way, I thought that the model_system_ref would be use for the global system, while this one for specific AtomsState or OrbitalsState.
There was a problem hiding this comment.
properties which are per atom or per orbital, while there might be a total one as well
Indeed, one can define orbitals at a system-wide level, or projected onto a specific ion, species, etc. This is why particle_indices alone don't suffice.
We have a
I would work towards a bi-directional reference, where electronic entities (like orbitals) are integrated as well.
I was testing this out in the other PR, already.
There was a problem hiding this comment.
I would develop anything based on how easy is to use, and leverage out logic to methods or references.
From the user perspective, is it easy to get a property and need to code something to read the particle indices to then get which atom refers to? And a similar question for the orbitals? In my opinion, the current entity_ref was the easiest solution. Equivalently, you can also leverage out the logic to resolve this kind of things to a method.
I think we are on the same page @ndaelman-hu, I just wanted to make sure we are all in the same one.
There was a problem hiding this comment.
Maybe just as a summary (also for myself), at the time of writing.
Either way, I thought that the model_system_ref would be use for the global system, while this one for specific AtomsState or OrbitalsState.
ModelSystem can point to a ParticleState or AtomsState at the top level alone (unsure whether programmatically enforced). The latter can point to an OrbitalState, so the connection is indirect at best.
There atm no way to define the OrbitalState (or SpinState) for a collection of particles / atoms. The latter is based on notes from a discussion with @EBB2675
From the user perspective, is it easy to get a property and need to code something to read the particle indices to then get which atom refers to? [...] In my opinion, the current entity_ref was the easiest solution.
I agree that direct references are generally indeed easier, but no comment on the use of particle_indices.
I think entity_ref is too limited, as it only allows me to reference an orbital / band, but it doesn't define the over which collection of particles / atoms.
I see 2 possible fixes. This tackles the same issue as Spin, so I suggest packing both together in a follow-up issue.
@ndaelman-hu Approximately when will you be tackling this part? We will plan some remaining parts of the QC schema accordingly with Denis |
Pull Request Test Coverage Report for Build 16906346307Details
💛 - Coveralls |
…operty`) - Apply ruff
|
All tests pass, some points that have been brought up (SCF, referencing species orbitals, spin) are deferred to their own issues. |
ndaelman-hu
requested review from
EBB2675,
JFRudzinski,
JosePizarro3 and
Copilot
There was a problem hiding this comment.
Pull Request Overview
This PR implements significant refactoring of the PhysicalProperty class and its integration with contributions, alongside comprehensive test updates and removal of deprecated patterns. The changes aim to simplify the codebase by consolidating functionality and improving plotting capabilities.
- Integration of
PhysicalContributionintoPhysicalPropertywith contributions as subsections - Addition of plotting functionality and sub-plot aggregation from contributions
- Removal of redundant
__init__methods and consolidation of name-setting logic
Reviewed Changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/nomad_simulations/schema_packages/physical_property.py | Major refactor integrating contributions, adding plotting support, and validation methods |
| src/nomad_simulations/schema_packages/variables.py | Consolidated name-setting logic in normalize method |
| src/nomad_simulations/schema_packages/properties/*.py | Removed redundant __init__ methods across all property classes |
| tests/test_physical_properties.py | Comprehensive new tests for plotting, contributions, and validation |
| tests/test_variables.py | Added test for name-setting during normalization |
| tests/test_outputs.py | Updated test expectations and removed deprecated test |
| tests/properties/test_*.py | Removed obsolete name assertion tests and fixed test logic |
Comments suppressed due to low confidence (1)
tests/properties/test_band_structure.py:115
- The return type change from Union[bool, tuple] to tuple | tuple[()] is inconsistent with the removal of the decorator. Consider using Optional[tuple[pint.Quantity, np.ndarray]] for clearer type annotation.
0.0,
Enhance `PhysicalProperty` description
…ad-simulations into physical-property-tweaking
As discussed, I broke off the modifications to
PhysicalPropertyin #197 (which was used to test plotting and mapping parser, as well as modifications tooutputs) into its own PR.This PR incrementally applies some modifications. If you disagree about any particular ones, mention the commit hash and we can see to cherry-pick it out. The rough order includes:
variables.PhysicalContributionintoPhysicalProperty. This aims to half the number of definitions for each property. There is a note for a follow-up PR, but we should consult Theodore here first.sub_plotsto manage aggregation of plots from contributions.@EBB2675 I would add the spin base section to a specialized version of
PhysicalProperty. Would leave this as a short follow-up.Similarly specializing the contributions for electronics requires the addition of
OrbitalsStatetoModelSystemitself, as I already raised.@everyone Feel free to raise suggestions for the remaining in-code comments.