Skip to content

Conversation

@ndaelman-hu
Copy link
Collaborator

While the rank / dimensionality is checked by nomad, the actual length along a specific axis typically isn't.
Atm, any length checks typically consist of the population of a n_ quantity and specific checks for some quantities.

Given the frequency of these checks, it would be better to off-load them to a specialized mechanism. The mechanism should still trigger during normalization (at the end?). It also makes most n_ quantities irrelevant, as they don't have any purpose in querying.

The PR takes a class decorator approach, for ease of collection and overview. While decorators are more modular, a compound behavior along inheritance will likely come across as most natural and less repetitive. In that case, a "removal" option is also necessary.

@ndaelman-hu ndaelman-hu requested review from Copilot and ladinesa May 28, 2025 12:53
@ndaelman-hu ndaelman-hu self-assigned this May 28, 2025
@ndaelman-hu ndaelman-hu added the improvement/fix Improvement or fix of a previous feature label May 28, 2025
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

Adds a class-decorator mechanism to consolidate shape validation across PhysicalProperty subclasses, replacing ad-hoc n_ length checks with a reusable system and enabling selective removal of checks.

  • Introduces accumulate_class_attributes, extract_shapes, and validate_shape_consistency utilities
  • Adds @same_shapes decorator for automatic post-normalization shape checks
  • Adds @remove_shape_checks decorator to override or disable inherited shape requirements
Comments suppressed due to low confidence (1)

src/nomad_simulations/schema_packages/physical_property.py:108

  • [nitpick] Add unit tests for same_shapes (and the complementary remove_shape_checks) to verify that shape requirements are correctly accumulated, validated, and removed across inheritance hierarchies.
cls._shape_requirements = accumulate_class_attributes(cls, '_shape_requirements', quantities)

@coveralls
Copy link

coveralls commented May 28, 2025

Pull Request Test Coverage Report for Build 15376478072

Details

  • 23 of 64 (35.94%) changed or added relevant lines in 2 files are covered.
  • 51 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-2.7%) to 65.03%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/nomad_simulations/schema_packages/physical_property.py 21 62 33.87%
Files with Coverage Reduction New Missed Lines %
src/nomad_simulations/schema_packages/physical_property.py 2 55.08%
src/nomad_simulations/schema_packages/variables.py 2 81.4%
src/nomad_simulations/schema_packages/properties/band_structure.py 47 40.0%
Totals Coverage Status
Change from base Build 15132670970: -2.7%
Covered Lines: 1854
Relevant Lines: 2851

💛 - Coveralls

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

improvement/fix Improvement or fix of a previous feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants