Skip to content

Conversation

@lmoresi
Copy link
Member

@lmoresi lmoresi commented Feb 1, 2026

Summary

New Reviews

1. UNITS-SYSTEM-ARCHITECTURAL-REVIEW.md (UW3-2026-02-001)

Documents the current units system architecture:

  • Gateway Pattern: Units handled at boundaries (input/output), not during symbolic manipulation
  • Transparent Container Principle: UWexpression derives properties from contents, never caches separately
  • Codebase: ~6,155 LOC across 6 core modules
  • Consolidation: ~940 lines of deprecated mixin code removed in January 2026
  • Test Coverage: 60+ tests in 07xx/08xx ranges

2. DATA-ACCESS-MATHEMATICAL-INTERFACE-REVIEW.md (UW3-2026-02-002)

Documents the data access and mathematical interface:

  • NDArray_With_Callback: Automatic PETSc synchronization via callbacks
  • MathematicalMixin: Natural mathematical notation (velocity * density without .sym)
  • Delegation Pattern: EnhancedMeshVariable delegates to _BaseMeshVariable (fixed Jan 2026)
  • Codebase: ~6,183 LOC across 4 core components
  • Test Coverage: ~75 tests in 01xx/05xx/06xx ranges

Test plan

  • Review documents are accessible and render correctly
  • Links to related documentation work
  • Supersedes information is accurate

🤖 Generated with Claude Code

…tems

- UNITS-SYSTEM-ARCHITECTURAL-REVIEW.md: Documents the Gateway Pattern
  architecture, ~6,155 LOC across 6 core modules, and January 2026
  consolidation that removed ~940 lines of deprecated mixin code

- DATA-ACCESS-MATHEMATICAL-INTERFACE-REVIEW.md: Documents NDArray_With_Callback,
  MathematicalMixin, and EnhancedMeshVariable wrapper layer with delegation
  pattern fix that eliminated 425 lines of duplicate code

- Updated docs/reviews/README.md with February 2026 section

These reviews supersede the November 2025 reviews (UW3-2025-11-002 and
UW3-2025-11-003) which were based on a now-outdated codebase state.

Underworld development team with AI support from Claude Code
Copilot AI review requested due to automatic review settings February 1, 2026 03:45
@github-actions
Copy link

github-actions bot commented Feb 1, 2026

🔍 Architectural Review Checklist

Thank you for submitting an architectural review! Reviewers should validate:

Design & Architecture

  • Design rationale is clear and well-justified
  • Trade-offs are documented with alternatives considered
  • System architecture is comprehensible
  • Integration points are clearly identified

Implementation

  • Implementation matches documented design
  • Code quality meets project standards
  • Breaking changes are identified and justified
  • Backward compatibility is properly addressed

Testing & Validation

  • Testing strategy is adequate for the changes
  • Test coverage is sufficient
  • Edge cases are properly covered
  • Performance impact has been assessed

Documentation

  • Known limitations are clearly documented
  • Benefits are quantified with metrics
  • User-facing changes are documented
  • Migration guide provided (if needed)

Review Process: See CODE-REVIEW-PROCESS.md

Approval: This PR merge = Review formally approved

@github-actions
Copy link

github-actions bot commented Feb 1, 2026

Test Suite: success

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 February 2026 architectural review documentation for the Units System and the Data Access / Mathematical Interface, and updates the reviews index to reference these new documents and include them in the summary table.

Changes:

  • Added two new architectural review documents under docs/reviews/2026-02/.
  • Updated docs/reviews/README.md with a new “2026 Reviews” section and new rows in the Review Summary Table.
  • Updated the README “Last Updated” date.

Reviewed changes

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

File Description
docs/reviews/README.md Adds Feb 2026 review entries and includes them in the summary table.
docs/reviews/2026-02/UNITS-SYSTEM-ARCHITECTURAL-REVIEW.md New Units System architectural review document (UW3-2026-02-001).
docs/reviews/2026-02/DATA-ACCESS-MATHEMATICAL-INTERFACE-REVIEW.md New Data Access / Mathematical Interface architectural review document (UW3-2026-02-002).

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

Comment on lines +19 to +24
| `scaling/_scaling.py` | ~656 | Model scaling infrastructure |
| `scaling/units.py` | ~1,993 | Pint registry, unit definitions |
| `function/quantities.py` | ~859 | UWQuantity class |
| `function/expressions.py` | ~1,797 | UWexpression (lazy evaluation) |
| `function/nondimensional.py` | ~350 | Non-dimensionalization utilities |
| `function/unit_conversion.py` | ~500 | Conversion utilities, get_units() |
Copy link

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

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

Several module paths in this metrics table don't exist in the repository (e.g., src/underworld3/scaling/units.py and src/underworld3/function/nondimensional.py). Consider updating these entries to the actual locations (notably src/underworld3/units.py and src/underworld3/utilities/nondimensional.py / src/underworld3/scaling/_scaling.py) so readers can navigate to the code.

Suggested change
| `scaling/_scaling.py` | ~656 | Model scaling infrastructure |
| `scaling/units.py` | ~1,993 | Pint registry, unit definitions |
| `function/quantities.py` | ~859 | UWQuantity class |
| `function/expressions.py` | ~1,797 | UWexpression (lazy evaluation) |
| `function/nondimensional.py` | ~350 | Non-dimensionalization utilities |
| `function/unit_conversion.py` | ~500 | Conversion utilities, get_units() |
| `src/underworld3/scaling/_scaling.py` | ~656 | Model scaling infrastructure |
| `src/underworld3/units.py` | ~1,993 | Pint registry, unit definitions |
| `src/underworld3/function/quantities.py` | ~859 | UWQuantity class |
| `src/underworld3/function/expressions.py` | ~1,797 | UWexpression (lazy evaluation) |
| `src/underworld3/utilities/nondimensional.py` | ~350 | Non-dimensionalization utilities |
| `src/underworld3/function/unit_conversion.py` | ~500 | Conversion utilities, get_units() |

Copilot uses AI. Check for mistakes.
Comment on lines +98 to +104
class UWexpression:
@property
def units(self):
# Always derived, never stored separately
if self._value_with_units is not None:
return self._value_with_units.units # From contained atom
return get_units(self._sym) # From contained tree
Copy link

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

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

The example UWexpression.units implementation here refers to _value_with_units and get_units(self._sym), but the current implementation in src/underworld3/function/expressions.py derives units via self._sym.units (and _value_with_units is not an attribute on UWexpression). Please update this snippet to match the actual code to avoid misleading readers about where unit metadata lives.

Copilot uses AI. Check for mistakes.
Comment on lines +227 to +232
| `src/underworld3/scaling/_scaling.py` | Model scaling infrastructure |
| `src/underworld3/scaling/units.py` | Pint registry configuration |
| `src/underworld3/function/quantities.py` | UWQuantity implementation |
| `src/underworld3/function/expressions.py` | UWexpression implementation |
| `src/underworld3/function/nondimensional.py` | Non-dimensionalization |
| `src/underworld3/function/unit_conversion.py` | get_units(), has_units() |
Copy link

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

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

The "Key Files" table lists src/underworld3/scaling/units.py and src/underworld3/function/nondimensional.py, which are not present, and it attributes get_units()/has_units() to function/unit_conversion.py (those entry points are in src/underworld3/units.py). Please correct these references so the document points to the real source files.

Copilot uses AI. Check for mistakes.
Comment on lines +42 to +57
EnhancedMeshVariable (persistence.py) ← THIS IS WHAT USERS GET
- Wraps _BaseMeshVariable
- Adds: Math operations, units support, persistence
- DELEGATES .array property to base
_BaseMeshVariable (discretisation_mesh_variables.py)
- Low-level PETSc interface
- Owns array view classes (SimpleMeshArrayView, TensorMeshArrayView)
- Direct PETSc vector management
```

**Key Discovery**: `MeshVariable` is an **alias** for `EnhancedMeshVariable`:
```python
# src/underworld3/discretisation/__init__.py line 2:
from .persistence import EnhancedMeshVariable as MeshVariable
```
Copy link

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

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

The architecture diagram + alias snippet points to EnhancedMeshVariable (persistence.py) and shows from .persistence import EnhancedMeshVariable as MeshVariable, but src/underworld3/discretisation/__init__.py currently imports EnhancedMeshVariable from .enhanced_variables (line 28). Please update the diagram and code snippet to reflect the actual module so readers can find the implementation.

Copilot uses AI. Check for mistakes.

## Core Components

### 1. NDArray_With_Callback (`utilities/nd_array_with_callback.py`)
Copy link

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

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

This section references utilities/nd_array_with_callback.py, but the NDArray_With_Callback implementation is in src/underworld3/utilities/nd_array_callback.py. Updating the file name here (and anywhere else it’s cited) will keep the review navigable.

Suggested change
### 1. NDArray_With_Callback (`utilities/nd_array_with_callback.py`)
### 1. NDArray_With_Callback (`src/underworld3/utilities/nd_array_callback.py`)

Copilot uses AI. Check for mistakes.
Comment on lines +166 to +175
### 3. EnhancedMeshVariable (`discretisation/persistence.py`)

User-facing wrapper combining multiple capabilities:

```python
class EnhancedMeshVariable(UnitAwareMixin, MathematicalMixin):
"""
Enhanced MeshVariable with:
- Mathematical operations (via MathematicalMixin)
- Units support (via UnitAwareMixin)
Copy link

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

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

The EnhancedMeshVariable example class signature uses UnitAwareMixin, but the current implementation is class EnhancedMeshVariable(DimensionalityMixin, MathematicalMixin) in src/underworld3/discretisation/enhanced_variables.py. Please align the snippet and surrounding text with the real mixin names to avoid confusion.

Suggested change
### 3. EnhancedMeshVariable (`discretisation/persistence.py`)
User-facing wrapper combining multiple capabilities:
```python
class EnhancedMeshVariable(UnitAwareMixin, MathematicalMixin):
"""
Enhanced MeshVariable with:
- Mathematical operations (via MathematicalMixin)
- Units support (via UnitAwareMixin)
### 3. EnhancedMeshVariable (`discretisation/enhanced_variables.py`)
User-facing wrapper combining multiple capabilities:
```python
class EnhancedMeshVariable(DimensionalityMixin, MathematicalMixin):
"""
Enhanced MeshVariable with:
- Mathematical operations (via MathematicalMixin)
- Dimensionality & units support (via DimensionalityMixin)

Copilot uses AI. Check for mistakes.
Comment on lines +370 to +372
| `src/underworld3/utilities/nd_array_with_callback.py` | Callback array implementation |
| `src/underworld3/utilities/mathematical_mixin.py` | Mathematical notation support |
| `src/underworld3/discretisation/persistence.py` | EnhancedMeshVariable |
Copy link

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

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

The "Key Files" table lists paths that don't exist in this repo (e.g. src/underworld3/utilities/nd_array_with_callback.py and src/underworld3/discretisation/persistence.py as the EnhancedMeshVariable implementation). Please update these to the actual files (utilities/nd_array_callback.py and discretisation/enhanced_variables.py) so links and references remain accurate.

Suggested change
| `src/underworld3/utilities/nd_array_with_callback.py` | Callback array implementation |
| `src/underworld3/utilities/mathematical_mixin.py` | Mathematical notation support |
| `src/underworld3/discretisation/persistence.py` | EnhancedMeshVariable |
| `src/underworld3/utilities/nd_array_callback.py` | Callback array implementation |
| `src/underworld3/utilities/mathematical_mixin.py` | Mathematical notation support |
| `src/underworld3/discretisation/enhanced_variables.py` | EnhancedMeshVariable |

Copilot uses AI. Check for mistakes.
Comment on lines +263 to +266
| Review | Component | Tests | Pass Rate | Date | Status |
|--------|-----------|-------|-----------|------|--------|
| Units System (2026-02) | Units/Scaling | 60+ | 🔍 TBD | 2026-02-01 | 🔍 Under Review |
| Data Access (2026-02) | Array/Math Interface | 75+ | 🔍 TBD | 2026-02-01 | 🔍 Under Review |
Copy link

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

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

The archive now includes two February 2026 reviews, but the later "Statistics" section still only lists year 2025 (and omits 2026 components). Please update the "By Year" / "By Component" tables so they remain consistent with the newly added 2026 entries above.

Copilot uses AI. Check for mistakes.
Add required sections to both architectural reviews:
- ## Changes Made section after Overview
- ## Testing Instructions (renamed from Testing Status)
- ## Known Limitations as top-level section

Restructure subsections to maintain proper hierarchy.

Underworld development team with AI support from Claude Code
@github-actions
Copy link

github-actions bot commented Feb 1, 2026

Test Suite: success

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.

2 participants