Replace GeometricSpace and Cell by Representation
#263
Conversation
…rdinates directly under `ModelSystem`
…and Representation classes - Removed length_vector_a/b/c and angle_vectors_b_c/a_c/a_b from both GeometricSpace and Representation classes - These properties were automatically computed from lattice vectors and not essential data - Updated assignment code to only set volume instead of length/angle measurements - Simplifies schema by removing redundant computed properties that can be derived from lattice_vectors
- Restructure lay-out `Representation` -- Make difference between hyper-volume and hyper-area more apparent
- Move out `from_ase_atoms` to produce components
- Leverage `ModelSystem.normalize` to simplify type signature - Explain architectural choices in pydocs string - Extend mapping to include velocities, fractional_coordinates, charges, volumes, atom labels
- Apply ruff
- Fix Optional parameter types in model_system.py (_copy_common_quantities) - Fix Optional parameter types in numerical_settings.py (None checks added) - Fix Optional parameter types in utils/utils.py (log decorator) - Replace AtomicCell with Representation in tests/utils/test_utils.py - Update test logic to use representations instead of cell subsection
ndaelman-hu
added
new feature
|
@JFRudzinski I would also like to add GlobalSymmnetry here, from #253 |
- Add backwards compatiblity layer for parsers
-- Patch `resolve_high_symmetry_points` and its tests
Pull Request Test Coverage Report for Build 19348454693Details
💛 - Coveralls |
|
@ndaelman-hu sorry i probably will not be able to look at this this week. |
There was a problem hiding this comment.
Pull Request Overview
This PR consolidates the previous GeometricSpace/Cell/AtomicCell structure into a unified Representation/RelativeRepresentation model and refactors ModelSystem to directly host original structural data, with alternate representations stored in ModelSystem.representations. It also introduces a standalone from_ase_atoms function and updates tests and utilities accordingly.
- Unifies structural abstractions under Representation and RelativeRepresentation, deprecating GeometricSpace/Cell/AtomicCell.
- Refactors ModelSystem.to_ase_atoms and adds standalone from_ase_atoms with extended ASE property mapping.
- Updates workflow, symmetry, numerical settings, and tests to use representations instead of cell(s).
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/workflow/test_equation_of_state.py | Migrated test to use representations instead of cell. |
| tests/utils/test_utils.py | Updated sibling resolution tests to use representations. |
| tests/test_model_system.py | Major updates to tests for new representation model and from_ase_atoms behavior. |
| tests/test_model_method.py | Adjusted tight-binding method tests to new Representation API. |
| tests/test_general.py | Partial migration; some legacy .cell usage remains. |
| tests/test_basis_set.py | Updated basis set tests to use representations. |
| tests/properties/test_spectral_profile.py | Adjusted spectral property tests to new Representation usage. |
| tests/conftest.py | Factory utilities updated to build ModelSystem with representations. |
| src/nomad_simulations/schema_packages/workflow/equation_of_state.py | Iteration over representations for volume extraction. |
| src/nomad_simulations/schema_packages/utils/utils.py | Type hint modernization and minor guard improvement. |
| src/nomad_simulations/schema_packages/numerical_settings.py | Switched from cell to representations; added guards. |
| src/nomad_simulations/schema_packages/model_system.py | Core refactor: removed GeometricSpace/Cell/AtomicCell, added Representation hierarchy, updated symmetry logic, added from_ase_atoms, and backward-compat aliases. |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| @log | ||
| def get_geometric_space_for_cell(self) -> None: | ||
| """ | ||
| Get the real space parameters for the cell using ASE. | ||
| Args: | ||
| logger (BoundLogger): The logger to log messages. | ||
| """ | ||
| logger = self.get_geometric_space_for_cell.__annotations__['logger'] | ||
|
|
||
| atoms = self.to_ase_atoms( | ||
| representation_index=0 if self.representations else None, logger=logger | ||
| ) | ||
| if atoms is None: | ||
| return # parent already logged the problem | ||
|
|
||
| try: | ||
| cell = atoms.get_cell() | ||
| if self.representations and len(self.representations) > 0: | ||
| cell_section = self.representations[0] | ||
| cell_section.volume = cell.volume * ureg.angstrom**3 | ||
| except Exception as exc: | ||
| logger.warning( | ||
| 'Failed to extract geometric-space data from ASE cell.', | ||
| exc_info=exc, | ||
| ) | ||
|
|
There was a problem hiding this comment.
get_geometric_space_for_cell now only sets volume on the first representation; previously available geometric metadata (vector lengths, angles, area/length for reduced dimensionality) is not computed here, leading to partial geometry population. Either extend this method to compute all supported geometric quantities (and for the selected representation index) or document that only volume is intentionally populated.
There was a problem hiding this comment.
@JFRudzinski do you think that we should retain quantities like vector length and angles? Note that they are also encoded in the lattice vectors. For cyrstals, the other symmetry descriptors also cover all the relevant angular information.
JFRudzinski
left a comment
JFRudzinski
left a comment
There was a problem hiding this comment.
Very nice! I like this new structure.
I see the back compatibility with cell, this is nice. Can you add an issue in the parser plugin branch to remove cell usage from all parsers, to make sure we tackle this at some point?
|
|
||
|
|
||
| class ModelSystem(System): | ||
| class ModelSystem(System, Representation): |
There was a problem hiding this comment.
for sure I do not understand the logic here but in general multiple inheritances is a symptom of not optimal design
There was a problem hiding this comment.
Yes, I'd prefer not using it either. Let's see:
Systemcomes from NOMAD, so the sub-optimal design are how definitions are split along the projects.Representationis just a way to avoid defining the same quantities twice. I worst case, it can be removed, though I would explore other options first.- Any specific tests I should run to see whether double inheritance causes an issue?
| ) | ||
| except Exception as e: | ||
| _logger.warning(f'{_exc_msg} {e}') | ||
| _logger.warning(f'{_exc_msg}', exc_info=e) |
There was a problem hiding this comment.
sorry you can undo this change as it appears it produced some unwanted failures in test, I can update this in a separate mr.
This reverts commit 3653c94.
-- TODO: stress test - Incorporate representation setup into `generate_model_system` - Remove defaults from `generate_model_system`, allowing for empty fields - Fix `test_model_system.py/test_normalize`
…-section # Conflicts: # src/nomad_simulations/schema_packages/model_system.py
…tional_cell During the merge from origin/develop, the variable name was updated from conventional_atomic_cell to conventional_cell to match the Representation refactoring, but one reference on line 546 was missed.
…ard compatibility - Remove AtomicCell and Cell class aliases - Remove cell SubSection and property redirect from ModelSystem - Update test_basis_set.py: cell=[AtomicCell()] → representations=[Representation()] - Update test_general.py: .cell.append() → .representations.append() Test results: 789 passed, 3 failed (99.6% pass rate)
- Remove unused 'type' parameter from generate_model_system() - Add 'representation_name' parameter to properly name representations - Update all callers: type='primitive' → representation_name='primitive' - Fix test_normalize: add is_representative=True and correct parameters - Fix parameter names: periodic_boundary_conditions → pbc, atomic_numbers → orbitals_symbols All 792 tests now passing (100%)
Resolved merge conflicts by integrating develop's new ElectronicState architecture with our Representation refactoring: - model_system.py: Updated normalization documentation to match develop's clearer version - conftest.py: Integrated develop's ElectronicState(basis_orbitals) pattern in generate_model_system(), replacing old OrbitalsState approach - test_model_method.py: Activated develop's test cases (10) with Representation architecture (replaced AtomicCell references) All tests passing (797/797).
|
@JFRudzinski I merged in the latest updates from |
This PR consolidates the cell representation architecture by replacing the deprecated
GeometricSpace,Cell, andAtomicCellclasses with a unifiedRepresentation+RelativeRepresentationaprpoach. While almost all quantities are retained, the structure now:ModelSystem(root) as the default representation, whereas alternative representations are stored underModelSystem.representations. This avoids unnecessary clutter for when there is only one representation.RealtiveRepresentationnow acts as a base class for defining various kinds of base sections, not just crystallographic ones.It also updates the type signature of
from_ase_atomsand makes it a standalone method. This simplifies the initiation ofModelSystems fromase.Atomsby providing a default clean starting point. The method';s mapping is also extended. These extensions only make sense in light ofRepresentationNOTE: it is strongly recommended to review this PR using semantic diff.