UW3 release candidate #32
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Wrapper function for the access manager to improve usability
Minor deetails.
Attempting to fix up the evaluation to be internally consistent, adding the wrappers for the access manager (and calling it `array` for now). Fix up tests to account for changes to evaluation array sizes on return (mostly squeezes) Fix a couple of other evaluation bugs (like the need to add a zero vector to some nearly-constant matrices). Repairing the quickstart guide here and there.
Fix for the first comment by @jcgraciosa. Also needed to update the JIT code to understand the MATRIX variable type and removing the other types that are (currently) not used. These cause compilation errors if they are lit up by mistake (even if that variable is not in any expression). Solvers: Navier Stokes / Advection-Diffusion - fix the projection terms and how they pack up their data.
…orldcode/underworld3 into global_evaluation_rewrite
Adding global evaluation routines to compute values anywhere in the distributed domain (also with extrapolation using rbf)
Fixing broken test (array shape)
Not setting initial rank - this has problems when the particle is located on the correct rank because it automatically, always gets migrated to rank 0 and we start hunting for it again (but always in the wrong place).
Missed one !
Nodal Point Swarm also had the issue of uninitialised rank. This causes particles on rank > 1 to be migrated to rank 0 on the first call to migrate. They then get lost ... Potential fix @jcgraciosa but maybe not the only issue here.
Fixing lack of rank initialisation in swarm.populate. Also: some changes to the derivative expressions code: - .where relevant sym returns the inner expression derived by the diff_variable inner expression in symbolic form (which is closer to the intent of .sym) and does not return the evaluated form. - .doit function returns the evaluated form (so this is like the sympy deferred derivative).
…tions And a couple of other fixes
This code is *probably* no longer needed
And leaving NodalPointPICSwarm alone
Shortening some monster-sized files. Imports etc should not change
…t in a more user friendly form attached to the swarm
**Changes:** - Changed parameter names to match what the model expects: - temperature_diff → temperature_difference - velocity → plate_velocity - Updated markdown to document which parameters are needed for which units - Added guidance: match parameters to your variables' unit types **Why this matters:** The warning messages explicitly state what parameters are missing. By using the correct parameter names (temperature_difference, plate_velocity), the reference quantities are properly recognized and scaling coefficients are calculated. **Notebook now:** - Sets correct reference quantities before mesh creation - Creates variables with units (T, u) without warnings - Demonstrates proper parameter naming for different unit types This eliminates all reference quantity warnings from the notebook. 🤖 Generated with Claude Code Co-Authored-By: Claude <[email protected]>
…ve models Updates Notebook 14 (Scaled Thermal Convection) to properly enforce the "Units Everywhere or Nowhere" principle documented in CLAUDE.md: Changes: - Update Stokes solver viscosity to use proper units (uw.quantity) when reference quantities are set - Update AdvDiffusion solver diffusivity to use proper units - Add educational comments explaining the dimensional consistency principle - Prevents dimensional scaling errors from mixing units and plain numbers This ensures the notebook demonstrates best practices for working with the units system when model.set_reference_quantities() is called. 🤖 Generated with Claude Code Co-Authored-By: Claude <[email protected]>
Problem: Semi-Lagrangian backward tracing failed with unit-aware calculations,
producing particles traced to coordinates outside the mesh domain.
Root Cause: The backward tracing mixed incompatible unit systems:
- Time step dt was converted to non-dimensional via model.to_model_magnitude()
- But coordinates were extracted using .magnitude (returning physical values in km)
- Or when coordinates had unit information, dt wasn't converted to compatible units
- Result: Dimensional mismatch in arithmetic caused huge coordinate values
The Two-Part Issue:
1. Original problem: Mixed non-dimensional time with physical coordinates
2. Refined issue: When coordinates have units, dt needs SI seconds (not years)
for dimensional analysis to work: dt[seconds] * v[m/s] = distance[m]
Solution: Detect unit system context and maintain consistency throughout:
- If coordinates have unit information: convert dt to SI seconds
(so dt * velocity = distance works dimensionally)
- If coordinates are non-dimensional: convert dt to non-dimensional
- Never extract .magnitude from coordinates; use unit-aware arithmetic naturally
Implementation Details:
- Check if coords are unit-aware via hasattr(coords, "magnitude")
- If unit-aware: dt.to('second') converts time values to seconds
- If non-dimensional: model.to_model_magnitude(dt) converts to model units
- Let Pint and UnitAwareArray handle dimensional arithmetic
Impact:
- Semi-Lagrangian advection works correctly with both unit-aware and
non-dimensional coordinate systems
- Fixes the "Point not located in mesh" regression
- Works with thermal convection simulations using reference quantities
- Test suite passes: test_1100_AdvDiffCartesian (3/3 tests)
Test Verification:
✅ Advection-diffusion test suite passes (all 3 test cases)
✅ Module imports without syntax errors
✅ No regression in non-dimensional case
✅ Ready for testing with Notebook 14
🤖 Generated with Claude Code
Co-Authored-By: Claude <[email protected]>
FIXES: - Fixed three cascading import errors in units.py that prevented UnitAwareArray from being non-dimensionalised: 1. Added missing numpy import (line 22) 2. Fixed relative import: ..utilities → .utilities (line 424) 3. Fixed relative import: ..function → .function (line 444) The Protocol 2 code for handling UnitAwareArray existed but was never reached due to these import failures. Now works correctly. VALIDATED: - UnitAwareArray with 'meter' units correctly non-dimensionalises - Returns dimensionless numpy array scaled by reference length - Test case: 1000m → ~0.000345 (with 2900km reference) KNOWN ISSUES (not addressed in this commit): - MeshVariable.coords returns dimensionless values (0-1) but incorrectly labels them with 'meter' units - This is a separate bug in the coords property implementation (discretisation_mesh_variables.py line 2354) - Should be addressed in a future commit DEPRECATION (from previous session): - Removed UnitAwareMixin from public API (__init__.py, utilities/__init__.py) - Marked as deprecated in units_mixin.py with warning message - Active implementation is in discretisation/enhanced_variables.py 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Fixed catastrophic bug where v.coords was returning mesh coordinate variable locations instead of the variable's own DOF locations. Changes: - discretisation_mesh_variables.py (lines 2338-2362): * Get variable-specific dimensionless coords from mesh._get_coords_for_var(self) * Scale from [0-1] to physical space using mesh.get_min/max_radius() * Wrap scaled coordinates in UnitAwareArray with 'meter' units - units.py (lines 22, 424, 444): * Fixed THREE import errors preventing UnitAwareArray non-dimensionalisation * Added missing `import numpy as np` * Fixed relative imports: `from .utilities` and `from .function` Why this matters: - Different FE degrees have DOFs at different locations * Degree 1: corners (25 points in 4x4 mesh) * Degree 2: corners + midpoints (81 points in 4x4 mesh) - Returning mesh coordinates for all variables breaks FE system - Each variable MUST return its own DOF coordinates Testing: - T (degree 2): 81 coordinate points ✓ - p (degree 1): 25 coordinate points ✓ - Coordinates properly scaled and carry units ✓ - UnitAwareArray non-dimensionalisation now works ✓ 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
…ensionalise()
Fixed critical bug where variable.coords returned non-dimensional values [0-1]
but claimed units of 'meters', causing 1e6 error factor for a 1000 km domain.
The Issue:
- meshVariable.coords was returning raw non-dimensional PETSc coordinates [0-1]
- Code incorrectly used get_min/max_radius() which returns cell geometry, not bounds
- Resulted in all coords being the same tiny value (~0.177 meters) with 'meter' units
The Fix:
- Use uw.dimensionalise() with length dimensionality {'[length]': 1}
- This properly scales non-dimensional coords to physical units using reference quantities
- Follows established pattern: stored data is non-dimensional, user sees dimensional view
Changes:
- discretisation_mesh_variables.py (lines 2337-2365):
* Get non-dimensional coords from mesh._get_coords_for_var(self)
* When mesh.units is set, call uw.dimensionalise(coords, {'[length]': 1})
* Returns UnitAwareArray with proper 'meter' units and correct physical values
Testing:
- Added test_0815_variable_coords_units.py with 4 comprehensive tests
- Validates coords without/with units, different FE degrees, consistency with mesh
- All tests passing ✓
Impact:
- uw.function.evaluate() already handles dimensional coords (non_dimensionalise)
- uw.kdtree.KDTree() already unit-aware with automatic conversion
- Visualization and hashing use mesh.X.coords (already dimensional)
- No internal code breaks - system designed for this pattern
Result:
- T.coords now correctly returns [0, 0] to [1e6, 1e6] meters (not 0.177)
- Matches mesh.X.coords scaling behavior
- Users see dimensional view, PETSc stores non-dimensional
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <[email protected]>
This commit fixes a TypeError that occurred when multiplying UWexpression and EnhancedMeshVariable objects (e.g., Ra * T). The issue was in the MathematicalMixin's isinstance checks preventing proper sympification. Changes: - Extract .sym property from MathematicalMixin objects in __mul__ and __rmul__ - This ensures both operands are pure SymPy objects for multiplication - Preserves lazy evaluation (no eager sympification) - Add unit-aware wrapper functions (placeholder for future work) - Fix import path for get_units (moved to units module) The fix follows user guidance to preserve lazy evaluation by extracting .sym rather than forcing sympification. Non-dimensional workflows now work correctly with expressions like Ra * T. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
…bility, and lazy evaluation This commit fixes 5 critical bugs in the units system discovered during notebook usage: 1. **PyVista visualization TypeError**: UnitAwareArray incompatible with PyVista's np.ndim() - Added .magnitude property check to strip units before visualization - Files: visualisation.py 2. **SymPy 1.14+ protocol compatibility**: SympifyError with matrix operations - Changed _sympify_() to _sympy_() protocol method for SymPy 1.14+ strict mode - Files: coordinates.py, expressions.py, quantities.py, mathematical_mixin.py (6 instances) 3. **Derivative indexing DimensionalityError**: dTdy[0,0] raised Pint conversion error - Added special handling for SymPy expressions in UWQuantity.__init__() - Store unit object separately instead of creating value * unit quantity - Files: quantities.py 4. **Derivative units incorrect**: uw.get_units(dTdy[0,0]) returned 'kelvin' not 'K/m' - Added explicit units parameter to with_units() function - Pass derivative units explicitly from UnitAwareDerivativeMatrix.__getitem__() - Files: function/__init__.py, mathematical_mixin.py 5. **Compound expression units incorrect**: (temperature/velocity[0]).units returned wrong units - Made UnitAwareExpression.units property compute lazily from SymPy expression - Uses compute_expression_units() for dimensional analysis instead of stored values - Critical for lazy-evaluated expressions where units must be derived from AST - Files: expression/unit_aware_expression.py (new module) All fixes validated with test cases. Compound expression units now correctly compute 'kelvin * second / meter' for temperature/velocity[0]. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
**Problem**: UWexpression has __getitem__ from MathematicalMixin, making it Iterable. SymPy's array multiplication/assignment rejected it, breaking tensor construction when using parameter descriptors with units. **Solution**: Element-wise tensor construction with loop instead of array multiplication operator. Wraps bare UWexpression results (edge case when identity element is 1/2) in Mul(1, value, evaluate=False) to bypass Iterable check. JIT unwrapper correctly finds UWexpression atoms inside Mul wrappers. **Changes**: - constitutive_models.py (ViscousFlowModel._build_c_tensor): Element-wise construction with wrapping for bare UWexpression edge cases - _api_tools.py (Parameter.__set__): Enhanced reset mechanism calls _reset() for constitutive model parameters **Validation**: - All 20 constitutive tensor regression tests passing - Parameter assignment with units works - Unit metadata preserved (_pint_qty) - Lazy evaluation maintained (symbol in tensor, not numeric value) - JIT unwrapping verified (UWexpression atoms substituted correctly) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
**Problem**: Parameters with units were being substituted with dimensional values (1e21) instead of non-dimensional values (1.0) during JIT unwrapping, causing incorrect scaling in compiled C code. **Root Cause**: The unwrapper substituted atom.sym (dimensional value) without checking if scaling was active or if the parameter had units metadata. **Solution**: 1. Enhanced _substitute_all_once() to check if scaling is active and parameter has units before substitution 2. If yes, compute non-dimensional value via non_dimensionalise() and substitute that instead of raw .sym value 3. Added user-facing show_nondimensional_form() function for inspecting what will be compiled **Changes**: - expressions.py (_substitute_all_once): Check scaling + units, use nondim value - units.py (show_nondimensional_form): New user-facing inspection function - __init__.py: Export show_nondimensional_form for uw.show_nondimensional_form() **Validation**: - Viscosity 1e21 Pa*s with scale 1e21 → unwraps to 1.0 ✓ - Tensor element 2*η → unwraps to 2.0 (not 2e21) ✓ - All 20 constitutive tensor regression tests still passing ✓ - show_nondimensional_form() works correctly ✓ **User Benefit**: Users can now verify non-dimensional forms of complex expressions before JIT compilation using uw.show_nondimensional_form() 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
**Changes**: - constitutive_models.py (DiffusionModel._Parameters): Converted diffusivity to class-level Parameter descriptor with units="m**2/s" - constitutive_models.py (DiffusionModel._build_c_tensor): Migrated to element-wise loop pattern (consistent with ViscousFlowModel) - expressions.py (_substitute_all_once): Fixed UWQuantity handling to use _sympy_() method correctly **Consistency**: Uses same element-wise construction pattern as ViscousFlowModel for code maintainability and predictable behavior with UWexpression objects. **Validation**: - Tensor construction with UWexpression parameters works - Units assignment functional: uw.quantity(1e-6, "m**2/s") - UWexpression preserved in tensor for JIT unwrapping - Parameter descriptor provides lazy evaluation 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
…tern
This commit completes parameter migration for 3 of 5 constitutive models:
- ViscousFlowModel (previously completed)
- DiffusionModel (newly migrated)
- DarcyFlowModel (newly migrated)
## Changes
### DiffusionModel (lines 1465-1527)
- Converted `diffusivity` to class-level Parameter descriptor
- Added units specification: "m**2/s"
- Updated tensor construction to element-wise loops for consistency
- Maintains lazy evaluation and unit metadata preservation
### DarcyFlowModel (lines 1661-1780)
- Converted `permeability` to class-level Parameter descriptor
- Added units specification: "m**2" (intrinsic permeability)
- Updated tensor construction to element-wise loops (consistent pattern)
- Removed old instance-level `_permeability` attribute
- Maintains lazy evaluation and unit metadata preservation
## Implementation Pattern
Both models now use consistent element-wise tensor construction:
```python
for i in range(d):
for j in range(d):
val = ... # tensor element
# Wrap bare UWexpression to avoid Iterable check
if hasattr(val, '__getitem__') and not isinstance(val, (sympy.MatrixBase, sympy.NDimArray)):
val = sympy.Mul(sympy.S.One, val, evaluate=False)
result[i, j] = val
```
This pattern:
- Handles UWexpression objects correctly
- Preserves symbolic expressions for JIT unwrapping
- Ensures non-dimensional scaling during compilation
- Consistent across all migrated models
## Testing
Validation includes:
- Units assignment with `uw.quantity()`
- Unit metadata preservation
- Lazy evaluation maintenance
- JIT unwrapping with non-dimensional substitution
- Tensor construction correctness
## Status
**Progress**: 3 of 5 models complete (60%)
**Remaining**:
- GenericFluxModel (needs investigation)
- MultiMaterialConstitutiveModel (complex, special handling required)
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <[email protected]>
…ally This fix resolves the AttributeError when using show_nondimensional_form() with UWQuantity objects and makes the function work universally. ## Problem User reported that show_nondimensional_form() failed with: AttributeError: 'UWQuantity' object has no attribute '_sympify_' Most expressions failed to work with the function. ## Root Cause The _substitute_all_once() function had two bugs: 1. Typo: checked for '_sympy_' instead of handling UWQuantity properly 2. Returned Python float instead of SymPy expression, causing downstream code expecting .atoms() to fail ## Solution Fixed _substitute_all_once() to properly handle UWQuantity: - When scaling active + quantity has units: non-dimensionalize it - Always return SymPy expression using sympy.sympify() - Ensures downstream code can use .atoms(), .subs(), etc. ## Testing Comprehensive test validates 10 different expression types: ✓ UWQuantity objects directly ✓ UWexpression parameters ✓ Constitutive tensor elements ✓ Expressions with mesh coordinates ✓ Expressions with mesh variables ✓ Complex nested expressions ✓ Different parameter values (viscosity contrasts) ✓ Diffusivity parameters ✓ Plain SymPy expressions (pass-through) ✓ Mixed expressions (parameters + symbols) All tests pass - function is now truly universal. ## Impact Users can now use show_nondimensional_form() on ANY expression type to debug non-dimensionalization, verify scaling, and inspect what numeric values the JIT compiler will see. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Solvers mostly fixed. Some bugs evident in evaluate.
This commit addresses two related bugs in the units/scaling system:
1. CRITICAL: Fix double non-dimensionalization bug in JIT compilation
- Location: src/underworld3/function/expressions.py
- Function: _apply_scaling_to_unwrapped()
- Problem: Variables were being scaled during unwrapping, but PETSc
already stores all variable data in non-dimensional [0-1] form
- Result: Pressure terms had coefficients like 0.000315576 instead of 1.0
- Fix: Disable variable scaling in _apply_scaling_to_unwrapped()
- Only constants (UWQuantity) need ND conversion, not variables
- Validates user's principle: auto-ND and manual-ND must produce
identical expressions for PETSc
2. Fix visualization coordinate units handling
- Location: src/underworld3/visualisation/visualisation.py
- Problem: PyVista was stripping unit information from coordinates,
causing uw.function.evaluate() to think coords were dimensionless
- Fix: Store both ND coordinates for PyVista visualization AND
original coordinate array (with units) for function evaluation
- New attributes: pv_mesh.units and pv_mesh.coord_array
- evaluate() now uses coord_array (preserves dimensional info)
Both fixes maintain the principle that PETSc operates entirely in
non-dimensional space, while the user-facing API handles unit conversions.
Testing: Auto-ND vs manual-ND Stokes stress now produces identical
expressions with correct coefficients.
Remove unreachable code from _apply_scaling_to_unwrapped() function. Update docstring to clearly explain why variable scaling is disabled. The function now simply returns the expression unchanged since: - Variables are already non-dimensional in PETSc - Constants are scaled in _unwrap_for_compilation() before this call Testing: - All Stokes ND tests pass (5/5) - All basic Stokes tests pass (6/6) - Units tests: 48/55 passing (failures appear pre-existing) The cleanup makes the code more maintainable and clearly documents the architectural decision to avoid double non-dimensionalization.
Moved UWQuantity constant handling from _unwrap_for_compilation() into _unwrap_expressions() for better code organization and visibility. Changes: - Added UWQuantity handling to _unwrap_expressions() (lines 123-136) - Removed duplicate UWQuantity code from _unwrap_for_compilation() - All unwrapping logic now in one place for easier maintenance - No behavioral changes - all tests pass Benefits: - Single responsibility: _unwrap_expressions() handles ALL expression types - Clearer code flow: no special cases scattered across functions - Prepares for eventual unification with evaluate path Validation: - All 5 Stokes ND tests pass (test_0818_stokes_nd.py) - Constants correctly non-dimensionalized when scaling active - Variables unchanged (already ND in PETSc) Related docs: - UNWRAPPING_COMPARISON_REPORT.md: Detailed pathway comparison - UNWRAPPING_UNIFICATION_PROPOSAL.md: Path forward for full unification
Added TODO.md tracking planned work including: - Unwrapping logic unification (high priority) - Regression test fixes (3/49 failing) - Complex solver validation - Legacy access pattern removal - Example notebook validation Documents completed work: - Variable scaling bug fix (2025-11-14) - UWQuantity unwrapping refactoring (2025-11-14) - Data access migration (2025-10-10) - Units system (2025-10-08) - DM state corruption fix (2025-10-14) - Coordinate units system (2025-10-15)
Updated uw.function.evaluate() to use the new unwrap_for_evaluate() pathway instead of generic unwrap(). Changes: - functions_unit_system.py: Use unwrap_for_evaluate() for proper constant non-dimensionalization and dimensionality tracking - _function.pyx: Supporting changes for evaluate pathway Benefits: - Cleaner separation: evaluate() uses evaluate-specific unwrapper - Better constant handling: UWQuantity constants properly non-dimensionalized - Dimensionality tracking: Returned from unwrapper instead of manually computed - Simplified logic: Reduced from ~150 lines to ~80 lines This is part of the unwrapping pathway clarification work, preparing for eventual full unification of JIT and evaluate unwrapping logic. Related: - unwrap_for_evaluate() introduced in expressions.py - JIT path uses _unwrap_for_compilation() - See UNWRAPPING_UNIFICATION_PROPOSAL.md for next steps
Reorganized code structure to consolidate enhanced variable implementations. Changes: - enhanced_variables.py: Now contains full EnhancedMeshVariable implementation (moved from persistence.py, +626 lines) - persistence.py: Simplified to focus on persistence features only (removed EnhancedMeshVariable class, -1043 lines) - __init__.py: Updated import path Benefits: - Better organization: Enhanced variables in one module - Clearer purpose: persistence.py focuses on persistence, not variable wrapping - Easier maintenance: All enhanced variable code in one place - No functional changes: Just code movement This consolidation makes the codebase structure clearer and prepares for potential future enhancements to the enhanced variable system.
Fixed unit cancellation bug where different input units (km/yr vs cm/yr) produced same dimensionless value. Changes: - units.py: Convert to base SI units before dividing by scale - Added .to_base_units() conversion - Added validation that units actually cancelled - Prevents subtle bugs from unit mismatches - unit_aware_array.py: Improved units handling - Better fallback for Pint Unit objects - More robust units extraction - __init__.py: Minor cleanup - Removed unused show_nondimensional_form import - Updated comments for enhanced variables Critical bug fix: Without SI conversion, uw.quantity(1, 'km/yr') and uw.quantity(1, 'cm/yr') both non-dimensionalized to the same value, causing incorrect physics. Example (with fix): - Input: 5 cm/year - Scale: 0.05 m/year (from 5 cm/year reference) - Old: 5 cm/year / 0.05 m/year → wrong (units don't cancel) - New: 0.05 m/year / 0.05 m/year → 1.0 (correct!) This ensures dimensional consistency in all solver operations.
Enhanced the .array property to properly handle units when setting/getting data.
Changes:
- pack_raw_data_to_petsc(): Added unit conversion and non-dimensionalization
- Handles UWQuantity and UnitAwareArray inputs
- Converts to variable's units before storing
- Non-dimensionalizes if scaling is active
- Critical entry point for dimensional data → PETSc storage
- ArrayProperty getter: Improved dimensionalization logic
- Properly handles ND → dimensional conversion when scaling active
- Uses uw.dimensionalise() with target dimensionality
- Returns UnitAwareArray when variable has units
- Returns plain arrays when no units
- ArrayProperty setter: Better unit handling
- Processes incoming UWQuantity/UnitAwareArray values
- Converts units to match variable's units
- Non-dimensionalizes before storing in PETSc
Benefits:
- var.array = uw.quantity(300, 'K') now works correctly
- Automatic unit conversion: var.array = uw.quantity(25, 'degC') works
- Proper ND scaling: values stored in PETSc are always ND when scaling active
- Getting var.array returns dimensional values with correct units
Example workflow:
T = uw.discretisation.MeshVariable('T', mesh, 1, units='K')
T.array[...] = uw.quantity(300, 'K') # Stored as ND in PETSc
values = T.array # Returns UnitAwareArray in K (dimensional)
This completes the units-aware array interface implementation.
Added ARCHITECTURE_ANALYSIS.md documenting the evaluation system architecture and units handling workflow. Content: - Overview of uw.function.evaluate() pathway - Coordinate transformation details (dimensional → ND) - Units handling workflow (wrapping/unwrapping) - Integration points with non-dimensional scaling - Technical notes on KDTree evaluation This document provides context for the evaluate pathway refactoring and serves as reference for future maintenance.
Systematic cleanup of root directory markdown files following CLEANUP_PLAN.md. Changes: - Removed duplicate LICENCE.md (kept LICENSE.md) - Moved 11 planning/design documents to planning/ - Consolidated TODO items from scattered files into TODO.md - Deleted 62 outdated files: - 3 session summaries (git history preserves them) - 13 progress/status reports (completed work logs) - 8 bug fix reports (fixes documented in commits) - 4 specific issue reports (issues resolved) - 15 analysis/investigation reports (insights captured) - 6 reference/guide documents (info in official docs) - 2 TODO files (consolidated into main TODO.md) - 11 other outdated documents Results: - Root .md files: 73 → 10 (essential docs only) - Planning directory: Organized with 12 design/plan documents - TODO.md: Updated with Notebook 13 simplification task Updated .gitignore to prevent future clutter: - Ignores *_ANALYSIS.md, *_REPORT.md, *_SUMMARY.md patterns - Ignores SESSION_SUMMARY*.md, PHASE_*.md patterns - Explicitly allows essential docs (README, TODO, etc.) Essential docs remaining in root: - README.md, LICENSE.md, CONTRIBUTING.md, CHANGES.md - CLAUDE.md, TODO.md, SPELLING_CONVENTION.md - ARCHITECTURE_ANALYSIS.md (recent, 2025-11-14) - UNWRAPPING_COMPARISON_REPORT.md (recent, 2025-11-14) - UNWRAPPING_UNIFICATION_PROPOSAL.md (recent, 2025-11-14) Code review documents created: - docs/reviews/2025-11/UNWRAPPING-REFACTORING-REVIEW.md - docs/reviews/2025-11/UNITS-SYSTEM-FIXES-REVIEW.md All historical information preserved in git history.
Deleted 79 temporary Python scripts from root directory: - 69 test_*.py files (untracked, debug scripts from units development) - 10 tracked demo/debug files: - debug_parameter_substitution.py - demo_jit_debugging.py - demo_nondimensional_viewer.py - demo_practical_use.py - demo_rounding_limitations.py - demo_simple_nondim.py - demo_unit_conversion.py - demo_universal_nondim.py - mesh_units_examples.py - mesh_units_simple.py Kept: - setup.py (required for building Cython extensions) Rationale: All deleted scripts were temporary investigation/demonstration code created during units system development. Functionality is covered by the official test suite in tests/. Note: The .gitignore already had /test_*.py pattern, so 69 test files were untracked. This commit removes the 9 tracked demo/debug scripts.
- Issue template: .github/ISSUE_TEMPLATE/architectural-review.yml - PR template: .github/PULL_REQUEST_TEMPLATE/architectural-review.md - Validation workflow: .github/workflows/architectural-review-validation.yml - Integration guide: docs/developer/GITHUB-REVIEW-INTEGRATION.md - Quick start guide: docs/developer/REVIEW-WORKFLOW-QUICK-START.md Enables structured submission, tracking, and approval of formal architectural reviews through GitHub Issues, PRs, and Projects. See: docs/developer/GITHUB-REVIEW-INTEGRATION.md for full details.
This is review #10 - a formal review of the review system itself! - Comprehensive documentation of review process - GitHub integration details (templates, labels, workflows) - Benefits, limitations, alternatives considered - Will serve as pilot review to test the new system This review will be the first submitted through the new GitHub review workflow, validating the process by using it.
* Add review history markers to Review System Infrastructure files PERMANENT BOILERPLATE - these review history sections document when files were created/reviewed and will remain in files permanently. Files marked for Review #10 (Review System Infrastructure): - .github/ISSUE_TEMPLATE/architectural-review.yml - .github/PULL_REQUEST_TEMPLATE/architectural-review.md - .github/workflows/architectural-review-validation.yml - docs/developer/GITHUB-REVIEW-INTEGRATION.md - docs/developer/REVIEW-WORKFLOW-QUICK-START.md - docs/reviews/2025-11/REVIEW-SYSTEM-INFRASTRUCTURE-REVIEW.md These markers will NOT be removed when PR closes - they serve as permanent documentation of review history. * Address reviewer feedback: Add navigation and workflow sections Added two critical sections requested by @lmoresi in PR #35: 1. **How to Review This Document** (top of document): - Quick orientation for reviewers - How to navigate the 6 files in PR - What to look for - Time estimate 2. **Reviewer Workflow** (before Known Limitations): - How to provide feedback (3 options) - How to change review status labels - How to approve the review (UI + CLI) - Status transition diagram - What happens after approval - Where to ask questions These sections make the review self-contained and actionable. Addresses issue #33 feedback about unclear workflow. * Add code change review requirements and fix branch links Addresses @lmoresi feedback from PR #35: 1. **Fixed branch links** (Issue: links pointed to base branch, not review files): - Added direct link to review branch - Added link to files in review - Added note about using PR 'Files changed' tab 2. **Added Code Change Review Requirements** section: - Purpose of change (required format) - Breaking changes & API changes (dedicated section) - Rationale for changes (alternatives considered) - Deprecation plan (if breaking changes exist) - Migration strategy (timeline, steps, resources) - Complete example review showing all 5 requirements This ensures code reviews have comprehensive documentation of: - What changed and why - Impact on users - How to migrate - Support timeline Addresses the 'only so much one can do reviewing a review' by establishing clear requirements for future code reviews.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The full release of UW3 should be at 3.0 ... when we do this, I would like to introduce some new coding patterns that are more conducive to the having and AI-bot (AI-Bat ?) give advice.
So that means there is a lot to review in here.