refactor(codegen): implement PRD-53 Phase 1 critical fixes - #149
Conversation
Phase 1 of the codegen pipeline audit (PRD-53):
1. Add missing [norm] sections to core algebras:
- euclidean2.toml, euclidean3.toml, projective2.toml, projective3.toml
- Explicitly sets primary_involution = "reverse" (the default)
- Makes intent clear and prevents silent bugs if defaults change
2. Remove dead conditional branches in traits.rs:
- Removed 13 instances of identical if/else branches
- Pattern: `if x.versor.is_some() { A } else { A }` -> `A`
- Reduces ~65 lines of redundant code
3. Remove constraint_simplify.rs stub module:
- Module was effectively dead code (constructor returned empty Vec)
- Removed ConstraintSimplifier from imports and usages in traits.rs
- Expressions now pass directly to ExpressionSimplifier
4. Fix TODO in types.rs for non-Euclidean metrics:
- Norm computation now properly handles metric signature:
- Positive metric (+1): add squared component
- Negative metric (-1): subtract squared component
- Zero metric (0): skip (degenerate, no contribution)
- norm() now uses abs().sqrt() for pseudo-Euclidean algebras
- Updated documentation to reflect metric-aware behavior
https://claude.ai/code/session_01E1TdEJtJHCHswwcXE8Ya5x
Phase 2 - Code Deduplication: - Extract compute_sandwich_expressions_impl for sandwich/antisandwich - Extract compute_projection_field_impl for project/antiproject - Reduces ~40 lines of duplicated code Phase 3 - Warning Suppression Removal: - Remove #[allow(clippy::vec_init_then_push)] by using vec![] initialization - Remove #[allow(clippy::type_complexity)] by adding ProductContribution and ProductContributions type aliases - Note: #[allow(...)] in generated code (quote! blocks) are necessary and intentionally kept Phase 4 - TOML Schema Documentation: - Create comprehensive docs/toml-schema.md with complete field reference - Document all sections: algebra, signature, norm, blades, types - Include field map syntax, semantic naming guidelines, and examples - Document automatic versor/sparse type detection - List validation rules Phase 5 - Extract Constants: - Add RUST_EDITION constant in format.rs Phase 6 - Field Naming: - Document semantic field naming conventions in toml-schema.md - Provide examples of good vs bad naming patterns https://claude.ai/code/session_01E1TdEJtJHCHswwcXE8Ya5x
Greptile OverviewGreptile SummaryThis PR implements Phase 1 of the codegen pipeline audit (PRD-53), focusing on critical fixes and dead code removal. The changes include adding explicit Key Changes
Note on PR DescriptionThe PR description states that norm computation "properly handles metric signature" with negative metric subtraction, but the implementation only adds zero-metric filtering. The code correctly computes coefficient-space Euclidean norms (always positive squared terms) rather than metric-aware norms (which would require sign handling for negative metrics). This is properly documented in the code comments but the PR description could be clearer about what was actually implemented. Confidence Score: 4/5
Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| let constructor = quote! { new_unchecked }; | ||
|
|
||
| // Compute norm squared based on metric signature. | ||
| // For each blade, we need to consider the metric. | ||
| // For now, this uses Euclidean norm (sum of squares). | ||
| // TODO: Handle non-Euclidean metrics properly (e.g., for CGA, PGA). | ||
| // For each blade, we consider the metric sign: | ||
| // - Positive metric (+1): add the squared component | ||
| // - Negative metric (-1): subtract the squared component | ||
| // - Zero metric (0): skip (degenerate components don't contribute) | ||
| // | ||
| // For Euclidean algebras, this gives the standard sum of squares. | ||
| // For pseudo-Euclidean algebras (Minkowski, etc.), this gives the | ||
| // proper spacetime interval. | ||
| let squared_terms: Vec<TokenStream> = ty | ||
| .fields | ||
| .iter() | ||
| .map(|field| { | ||
| .filter_map(|field| { | ||
| let name = format_ident!("{}", field.name); | ||
| let blade = Blade::from_index(field.blade_index); | ||
| let metric = self.blade_metric(blade); | ||
|
|
||
| if metric >= 0 { | ||
| quote! { self.#name * self.#name } | ||
| } else { | ||
| // Negative metric: subtract instead of add | ||
| // Note: we still compute the absolute value for norm purposes | ||
| quote! { self.#name * self.#name } | ||
| match metric.cmp(&0) { | ||
| std::cmp::Ordering::Greater => { | ||
| // Positive metric: add the squared term | ||
| Some(quote! { self.#name * self.#name }) | ||
| } | ||
| std::cmp::Ordering::Less => { | ||
| // Negative metric: subtract the squared term | ||
| Some(quote! { -self.#name * self.#name }) | ||
| } | ||
| std::cmp::Ordering::Equal => { | ||
| // Zero metric (degenerate): no contribution to norm | ||
| None | ||
| } | ||
| } | ||
| }) | ||
| .collect(); | ||
|
|
||
| // Handle the edge case where all components have zero metric | ||
| let norm_squared_body = if squared_terms.is_empty() { | ||
| quote! { T::zero() } | ||
| } else { | ||
| quote! { #(#squared_terms)+* } | ||
| }; | ||
|
|
||
| quote! { | ||
| /// Returns the squared Euclidean norm. | ||
| /// Returns the squared norm using the algebra's metric. | ||
| /// | ||
| /// This is the sum of squares of all components. | ||
| /// For Euclidean algebras, this is the sum of squares. | ||
| /// For pseudo-Euclidean algebras (Minkowski, etc.), components with | ||
| /// negative metric are subtracted, giving the proper spacetime interval. | ||
| /// Components with zero metric (degenerate bases) do not contribute. | ||
| /// | ||
| /// Note: For pseudo-Euclidean algebras, this may return negative values | ||
| /// for spacelike elements. | ||
| #[inline] | ||
| pub fn norm_squared(&self) -> T { | ||
| #(#squared_terms)+* | ||
| #norm_squared_body | ||
| } | ||
|
|
||
| /// Returns the Euclidean norm. | ||
| /// Returns the norm (square root of the absolute value of norm_squared). | ||
| /// | ||
| /// For pseudo-Euclidean algebras where norm_squared can be negative, | ||
| /// this returns the square root of the absolute value. | ||
| #[inline] | ||
| pub fn norm(&self) -> T { | ||
| self.norm_squared().sqrt() | ||
| self.norm_squared().abs().sqrt() |
There was a problem hiding this comment.
Check that generated algebras were regenerated after codegen changes. Per the review checklist: "All algebras regenerated after changes". Run:
for toml in crates/clifford-codegen/algebras/*.toml; do
cargo run --package clifford-codegen -- generate "$toml" --force
done
git diff --exit-code src/specialized/*/generated/Context Used: Context from dashboard - Review context (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/clifford-codegen/src/codegen/types.rs
Line: 667:731
Comment:
Check that generated algebras were regenerated after codegen changes. Per the review checklist: "All algebras regenerated after changes". Run:
```bash
for toml in crates/clifford-codegen/algebras/*.toml; do
cargo run --package clifford-codegen -- generate "$toml" --force
done
git diff --exit-code src/specialized/*/generated/
```
**Context Used:** Context from `dashboard` - Review context ([source](https://app.greptile.com/review/custom-context?memory=97b93c64-e03c-4a4e-a5ae-014a67e06ddf))
How can I resolve this? If you propose a fix, please make it concise.Address Greptile review feedback: - Revert metric-aware norm to Euclidean norm (sum of squares) - The metric-based sign changes broke complex numbers where i^2 = -1 - Keep the zero-metric skip for degenerate bases (PGA e0) - Regenerate all specialized algebras after codegen changes The proper metric-aware norm would need involution-based computation (x * involute(x)), which is a separate feature tracked in the [norm] section's primary_involution field. https://claude.ai/code/session_01E1TdEJtJHCHswwcXE8Ya5x
- Apply cargo fmt to codegen source files - Regenerate all algebras after codegen changes - Updated norm documentation to clarify coefficient-space vs metric-aware norms Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
Algebras have been regenerated. Applied formatting fixes and regenerated all algebras after codegen changes. |
|
@greptileai review |
| // Negative metric: subtract instead of add | ||
| // Note: we still compute the absolute value for norm purposes | ||
| quote! { self.#name * self.#name } | ||
| Some(quote! { self.#name * self.#name }) |
There was a problem hiding this comment.
PR description claims norm computation handles negative metric by subtracting, but code only adds positive squared terms regardless of metric sign.
The PR description states:
- Positive metric (+1): add squared component
- Negative metric (-1): subtract squared component
But line 689 always generates self.#name * self.#name (addition). For negative metrics, this should generate a subtraction to properly compute the metric-aware norm squared.
For Minkowski space (signature +--), the norm squared should be t*t - x*x - y*y, not t*t + x*x + y*y.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/clifford-codegen/src/codegen/types.rs
Line: 689:689
Comment:
PR description claims norm computation handles negative metric by subtracting, but code only adds positive squared terms regardless of metric sign.
The PR description states:
- Positive metric (+1): add squared component
- Negative metric (-1): subtract squared component
But line 689 always generates `self.#name * self.#name` (addition). For negative metrics, this should generate a subtraction to properly compute the metric-aware norm squared.
For Minkowski space (signature +--), the norm squared should be `t*t - x*x - y*y`, not `t*t + x*x + y*y`.
How can I resolve this? If you propose a fix, please make it concise.
2 participants
Phase 1 of the codegen pipeline audit (PRD-53):
Add missing [norm] sections to core algebras:
Remove dead conditional branches in traits.rs:
if x.versor.is_some() { A } else { A }->ARemove constraint_simplify.rs stub module:
Fix TODO in types.rs for non-Euclidean metrics:
https://claude.ai/code/session_01E1TdEJtJHCHswwcXE8Ya5x