Make the Algebra and Module traits multigrading-capable - #268
Open
JoeyBF wants to merge 2 commits into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (40)
📝 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://cdn.github.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
JoeyBF
force-pushed
the
claude/259-phase1-multigrade
branch
from
July 13, 2026 06:53
df73512 to
a6b6947
Compare
Collaborator
Author
|
Pushed
I did not add the suggested output validation to Context: this PR is deliberately just the multigrading groundwork; the Generated by Claude Code |
This was referenced Aug 1, 2026
JoeyBF
force-pushed
the
claude/259-phase1-multigrade
branch
from
August 1, 2026 06:26
a6b6947 to
f96524d
Compare
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
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. - Also corrects a pre-existing latent bug in the `sq0` example: `DoubleModule` (which represents degree 2n) forwarded the full degree to the inner module in `element_to_string`, unlike every sibling method; now halves it like `basis_element_to_string`. 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
JoeyBF
force-pushed
the
claude/259-phase1-multigrade
branch
from
August 1, 2026 07:09
f96524d to
ae0b613
Compare
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
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