Make the Algebra and Module traits multigrading-capable#268
Conversation
Generalize the `Algebra` trait family to be multigrading-capable, as the foundation for treating Ext(k,k) as a genuine bigraded algebra (issue SpectralSequences#259). - `Algebra<const N: usize = 1>`: degree *inputs* now take `impl Into<MultiDegree<N>>`, so singly-graded callers keep passing bare `i32`s (via new `From<i32> for MultiDegree<1>`). The default `N = 1` keeps every existing `A: Algebra` bound and `dyn` usage working unchanged. - `enum_dispatch` cannot handle a generic trait, so `Algebra` is removed from `SteenrodAlgebra`'s dispatch list and hand-rolled via `dispatch_steenrod!` (the pattern already used for `PairAlgebra`). `UnstableAlgebra`, `GeneratedAlgebra`, and `MuAlgebra` stay singly-graded and keep enum_dispatch. - Milnor, Adem, and Field implement `Algebra` (default `N = 1`), converting the incoming degree to `i32` at each method boundary. - Degree-returning methods stay `i32` (the distinguished filtration direction), so the resolution engine's degree arithmetic is untouched. - `algebra` now depends on `sseq` for `MultiDegree` (no dependency cycle). Behavior is unchanged for the singly-graded path; all 51 algebra tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Nk9J57zb6GpvZxrduhGSdY
|
Warning Review limit reached
Next review available in: 37 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (32)
📝 WalkthroughWalkthroughThe algebra crate adopts const-generic ChangesMulti-degree API migration
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ext/crates/algebra/src/module/module_trait.rs (1)
67-148: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winValidate the target degree before promising a non-panicking action.
Both methods delegate without computing the target basis or validating
resultagainstop_degree + input_degree. A valid source/index with a mismatched output slice can therefore still panic inside implementations.Proposed validation
+ let output_degree = op_degree + mod_degree; + self.compute_basis_multi(output_degree); + let output_dim = self.dimension_multi(output_degree); + if result.as_slice().len() != output_dim { + return Err(ActError::InvalidInput(format!( + "result length {} does not match module dimension {output_dim} in degree {output_degree}", + result.as_slice().len() + ))); + }Apply the equivalent check using
op_degree + input_degreeintry_act_multi.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ext/crates/algebra/src/module/module_trait.rs` around lines 67 - 148, Update try_act_multi to compute and validate the target degree, op_degree + input_degree, before delegating to act_multi: ensure the target degree is supported and result has the required target dimension, returning the appropriate ActError instead of allowing implementation panics. Apply the equivalent target-basis and result validation in try_act_on_basis_multi using op_degree + mod_degree before calling act_on_basis_multi.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ext/crates/algebra/src/module/module_trait.rs`:
- Around line 153-158: Update the default Module trait method is_unit to return
false whenever the grading has more than one component (N > 1), before applying
the existing degree and dimension checks. Retain the current checks for the
single-component case, allowing multigraded implementations to provide accurate
behavior by overriding is_unit.
---
Outside diff comments:
In `@ext/crates/algebra/src/module/module_trait.rs`:
- Around line 67-148: Update try_act_multi to compute and validate the target
degree, op_degree + input_degree, before delegating to act_multi: ensure the
target degree is supported and result has the required target dimension,
returning the appropriate ActError instead of allowing implementation panics.
Apply the equivalent target-basis and result validation in
try_act_on_basis_multi using op_degree + mod_degree before calling
act_on_basis_multi.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 126d7619-142a-4309-b461-21e59fc2559b
📒 Files selected for processing (38)
ext/crates/algebra/Cargo.tomlext/crates/algebra/src/algebra/adem_algebra.rsext/crates/algebra/src/algebra/algebra_trait.rsext/crates/algebra/src/algebra/field.rsext/crates/algebra/src/algebra/milnor_algebra.rsext/crates/algebra/src/algebra/steenrod_algebra.rsext/crates/algebra/src/module/finite_dimensional_module.rsext/crates/algebra/src/module/finitely_presented_module.rsext/crates/algebra/src/module/free_module.rsext/crates/algebra/src/module/hom_module.rsext/crates/algebra/src/module/homomorphism/free_module_homomorphism.rsext/crates/algebra/src/module/homomorphism/full_module_homomorphism.rsext/crates/algebra/src/module/homomorphism/hom_pullback.rsext/crates/algebra/src/module/homomorphism/mod.rsext/crates/algebra/src/module/homomorphism/quotient_homomorphism.rsext/crates/algebra/src/module/mod.rsext/crates/algebra/src/module/module_trait.rsext/crates/algebra/src/module/quotient_module.rsext/crates/algebra/src/module/rpn.rsext/crates/algebra/src/module/suspension_module.rsext/crates/algebra/src/module/tensor_module.rsext/crates/sseq/src/coordinates/degree.rsext/examples/bruner.rsext/examples/ext_m_n.rsext/examples/lift_hom.rsext/examples/resolution_size.rsext/examples/sq0.rsext/examples/steenrod.rsext/src/chain_complex/chain_homotopy.rsext/src/chain_complex/finite_chain_complex.rsext/src/chain_complex/mod.rsext/src/nassau.rsext/src/resolution.rsext/src/resolution_homomorphism.rsext/src/secondary.rsext/src/yoneda.rsext/tests/extend_identity.rsext/tests/non_zero_min_degree.rs
Second half of the multigrading foundation for issue SpectralSequences#259: generalize the `Module` trait to bigraded (and N-graded) modules, so Ext(M, k) can be a genuine `Module` over the ring Ext(k, k). - `Module<const N: usize = 1>` with `type Algebra: Algebra<N>`. The default `N = 1` keeps every existing `M: Module` bound and the `dyn Module` / `SteenrodModule` alias working unchanged. - `Module` must stay object-safe (`SteenrodModule = Arc<dyn Module>`), so its degree-taking methods take concrete `MultiDegree<N>` and are suffixed `_multi` (e.g. `dimension_multi`, `act_on_basis_multi`). A blanket `ModuleExt` trait provides the canonical ergonomic names (`dimension`, `act_on_basis`, …) taking `impl Into<MultiDegree<N>>`, forwarding to the core methods. It is implemented for every module including `dyn Module`, so callers keep passing bare `i32`s. - All eight module implementors (free, FD, FP, hom, quotient, RPn, suspension, tensor) implement the `_multi` methods, converting to `i32` at the boundary. - Degree-returning methods (`min_degree`, `max_degree`, `max_computed_degree`) stay `i32` (the distinguished filtration direction), so the resolution engine's degree arithmetic is untouched. - Downstream call sites (resolution, nassau, yoneda, secondary, chain complexes, examples, tests) gain a `use algebra::module::ModuleExt` import; no logic changes. Behavior is unchanged for the singly-graded path: all algebra and ext tests pass (the one failing test, `test_tempdir_lock`, is a pre-existing environment issue about read-only file permissions under root, unrelated to this change). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Nk9J57zb6GpvZxrduhGSdY
df73512 to
a6b6947
Compare
|
Pushed
I did not add the suggested output validation to Context: this PR is deliberately just the multigrading groundwork; the Generated by Claude Code |
2 participants
What & why
Generalize the
Algebra/Moduletrait family to be multigrading-capable, so a bigraded object (likeExt) can implement them directly. This is groundwork for #259 (splittingExtAlgebrainto a ringExt(k, k)and a moduleExt(M, k)that are anAlgebraand aModule); on its own it is a pure generalization with zero behavior change for all existing single-graded code.The grading types already exist in the
sseqcrate (MultiDegree<N>, withBidegree = MultiDegree<2>); this PR just makes the traits consume them.How
trait Algebra<const N: usize = 1>andtrait Module<const N: usize = 1>. The defaulted const generic means every existingA: Algebra/M: Modulebound and theArc<dyn Module>alias keep working unchanged — only future bigraded code writes<2>.impl Into<MultiDegree<N>>. WithFrom<i32> for MultiDegree<1>, every existingi32call site (~350 of them) compiles unchanged; the method.into()s at the top. Degree returns stayi32(the distinguished filtration direction), so the resolution engine's degree arithmetic is entirely untouched.Moduleis used asdyn Module, andimpl Into<…>arguments are not object-safe. So the object-safe core methods take a concreteMultiDegree<N>and are suffixed_multi(dimension_multi,act_on_basis_multi, …); a blanketModuleExtextension trait re-exposes the ergonomic names (dimension,act_on_basis, …) takingimpl Into<MultiDegree<N>>, and works even ondyn Module. Downstream code just gainsuse algebra::module::ModuleExt.Algebrais removed fromSteenrodAlgebra's#[enum_dispatch(...)]list and hand-dispatched via the existingdispatch_steenrod!macro (the same patternPairAlgebraalready uses for its associated type).UnstableAlgebra/GeneratedAlgebra/MuAlgebrastay singly graded and keep#[enum_dispatch].Only the ~3 concrete algebras (Milnor, Adem, Field) and ~8 module implementors gain
impl Into+ a one-line.into()per method; the bulk of the diff is those mechanical signature updates plusModuleExtimports at call sites.Testing
cargo test --features concurrent --lib --tests --workspace,cargo test --examples, and doctests: green (the lonetest_tempdir_lockfailure is a pre-existing sandbox/permissions artifact, unrelated — it fails onmastertoo).cargo fmt --check, the clippy matrix (--no-default-features/--all-targets,-D warnings), and rustdoc (-D warnings, private items, all features): clean.Follow-ups
The actual #259 split (
ExtAlgebraring /ExtModulemodule implementing these traits) builds on this and will come as a separate PR, followed by the secondary-layer split. This PR intentionally does not close #259.🤖 Generated with Claude Code
https://claude.ai/code/session_01Nk9J57zb6GpvZxrduhGSdY
Generated by Claude Code
Summary by CodeRabbit