Split ExtAlgebra into a ring ExtAlgebra and a module ExtModule - #277
Open
JoeyBF wants to merge 4 commits into
Open
Split ExtAlgebra into a ring ExtAlgebra and a module ExtModule#277JoeyBF wants to merge 4 commits into
JoeyBF wants to merge 4 commits into
Conversation
|
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 Plus Run ID: 📒 Files selected for processing (47)
📝 WalkthroughWalkthroughThis PR generalizes algebra and module APIs to ChangesMultidegree API
Ext algebra and module abstractions
Examples
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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: 4
🤖 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/examples/sq0.rs`:
- Around line 287-288: Update element_to_string_multi to pass the halved degree
to the inner module, matching basis_element_to_string_multi and DoubleModule’s
2n representation; use the converted degree value when calling the inner
formatter so it receives n rather than 2n.
In `@ext/src/ext_algebra/massey.rs`:
- Around line 63-72: The documentation for massey_b_hom in
ext/src/ext_algebra/massey.rs lines 63-72 must qualify the caching behavior:
generator product maps are cached and shared, while multi-generator classes
assemble a fresh combined map per call. Update the related documentation at
ext/src/ext_algebra/massey.rs lines 294-296 with the same qualification; no code
changes are required.
In `@ext/src/ext_algebra/secondary.rs`:
- Around line 43-49: Rename the public type SecondaryExtAlgebra to
SecondaryExtModule throughout the implementation, including both impl blocks,
its module documentation, all constructors, and every construction site such as
examples and SecondaryExtAlgebra::new. Preserve the existing behavior and ensure
references use the renamed type consistently.
In `@ext/src/resolution_homomorphism.rs`:
- Around line 174-177: Enforce the documented empty-name precondition at the
start of linear_combination by asserting that name is empty, so invalid callers
fail immediately before maps or save state are populated.
🪄 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 Plus
Run ID: 1edd1ab1-6e85-4d4c-933f-12dd25c5be8e
📒 Files selected for processing (46)
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/massey.rsext/examples/product.rsext/examples/resolution_size.rsext/examples/secondary.rsext/examples/secondary_product.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/ext_algebra/massey.rsext/src/ext_algebra/mod.rsext/src/ext_algebra/secondary.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.rsweb_ext/sseq_gui/src/actions.rs
JoeyBF
force-pushed
the
claude/259-phase2-ext-split
branch
from
August 1, 2026 06:28
5adc5f1 to
8ef36da
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-phase2-ext-split
branch
from
August 1, 2026 07:10
8ef36da to
4c96f46
Compare
…equences#259) Split the conflated `ExtAlgebra` into two objects that match the two things a resolution computes: - `ExtAlgebra` is the ring `Ext(k, k)`, backed by a resolution of `k`. It owns the ring-product cache (`res(k) -> res(k)`) and is the single home for the multiply-by-a-class maps Massey products need (`ExtAlgebra::class_product_map`), fixing the awkwardness noted in SpectralSequences#258 where `massey_b_hom` rebuilt that map from scratch each call. - `ExtModule` is a module `Ext(M, k)` over that ring, backed by a resolution of `M`. It holds a shared `Arc<ExtAlgebra>` (so all modules over the same `k` reuse one ring cache) and its own module-action cache (`res(M) -> res(k)`). `M == k` collapses to "a module over itself": `ExtModule` shares its resolution `Arc` with the ring, so `is_unit` is now a structural `ptr_eq` (`ExtModule::is_unit`) rather than a stored flag. `without_unit` is replaced by `ExtModule::intrinsic` for the resolution-intrinsic `d2` path. `ExtAlgebra` implements `algebra::Algebra<2>` and `ExtModule` implements `algebra::module::Module<2>` (with `Algebra = ExtAlgebra`), realising the "resolving an algebra yields an Algebra, resolving a module yields a Module" correspondence on the real trait family. The trait product/action methods are total; the inherent `multiply_into`/`try_multiply` keep their `Option`-returning out-of-range ergonomics. The shared product core lives in module-level helpers (`cached_generator_product_map`, `products_into`, `combine_product`) so ring and module products do not duplicate logic. Massey products move to `ExtModule` (a, b in the ring; c in the module), with `massey_b_hom` now sourced from the shared ring cache via `class_product_map`. `SecondaryExtAlgebra` wraps an `ExtModule`; the four examples and in-module tests are updated. Behaviour is unchanged: the three Massey regression tests (SpectralSequences#116), the product test, and the d2 test all pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Nk9J57zb6GpvZxrduhGSdY
Add `ResolutionHomomorphism::linear_combination`, which builds the chain map `∑ cᵢ·fᵢ` from maps sharing a source, target, and shift by summing their generator images directly — no quasi-inverse lift. `ExtAlgebra::class_product_map` now uses it for multi-generator classes: a single generator returns the cached per-generator map, and a general class *adds* the cached generator maps instead of re-solving the class from scratch via `from_class`. This realises the "storing lifts per generator, adding maps is cheap" design and gives the shared ring cache a real payoff for the multiply-by-a-class maps Massey products consume. A new regression test cross-checks the two independent linear-combination strategies at the first two-generator bidegree of Ext(F₂,F₂) — (n=15, s=5): `class_product_map` sums maps at the chain level, `multiply_into` sums them at the hom_k level, and the induced products must agree. They do. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Nk9J57zb6GpvZxrduhGSdY
JoeyBF
force-pushed
the
claude/259-phase2-ext-split
branch
from
August 1, 2026 07:31
4c96f46 to
285e8bc
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.
Closes #259.
What
ExtAlgebratoday conflates two different objects: the ringis_unitflag and awithout_unitconstructor. This PR splits them into two types that implement the actualalgebra::Algebra/algebra::Moduletraits:ExtAlgebra— the ringclass_product_map).impl Algebra<2> for ExtAlgebra.ExtModule— the moduleArc<ExtAlgebra>, plus its own module-action cache.impl Module<2> for ExtModulewithtype Algebra = ExtAlgebra."An algebra is a module over itself" makes the$M = k$ case structural: it's an
ExtModulesharing the ring'sArc, andis_unitis justArc::ptr_eq.without_unitbecomesExtModule::intrinsic.The bigrading is why this is possible now: Ext is bigraded$(s,t)$ , so it needs
Algebra<2>/Module<2>. That capability is the previous PR in the stack (#268), which makes theAlgebra/Moduletrait family multigrading-capable with zero behavior change for the singly-graded engine.Also in this PR
ResolutionHomomorphism::linear_combination— a cheap primitive that assembles a multiply-by-a-class chain map by adding the cached per-generator maps (no quasi-inverse solves). It backs the trait product methods and is cross-validated againstmultiply_intoby a new test.Notes for review
Optionhelpers stay inherent.Algebra::multiply_basis_elements/Module::act_on_basispanic if the bidegree isn't resolved (the trait contract — callcompute_basisfirst). The existingOption-returningmultiply_into/try_multiplyremain as inherent helpers for the "maybe out of computed range" ergonomics the examples rely on.ExtAlgebrainto a ringExtAlgebra(over k) and anExtModule(over M) #259.min_degree/max_computed_degreereport the filtrationsaxis (bounded-below direction) rather thant; documented at each site, consistent with the "degree-returns stayi32= filtration" convention from Make the Algebra and Module traits multigrading-capable #268.Stacking
This is built on #268 (the multigrade trait work). Until #268 merges, this PR's diff includes those commits too; it will collapse to just the ext-split commits once #268 lands and I rebase.
Testing
cargo test -p ext ext_algebra(10 tests incl. the #116 Massey regressions,class_product_mapvsmultiply_into, a trait-surface smoke test, anjust lint/just testCI-parity matrix, all green.🤖 Generated with Claude Code
https://claude.ai/code/session_01Nk9J57zb6GpvZxrduhGSdY
Generated by Claude Code
Summary by CodeRabbit