Multi indexed migration - #218
Conversation
📝 WalkthroughWalkthroughThis PR refactors the spectral sequence module by replacing Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ext/crates/once/src/multiindexed/mod.rs`:
- Line 1: The file imports std::sync::atomic::AtomicI32 directly which bypasses
the crate's loom-compatible atomics; change the import to use the crate
compatibility layer (crate::std_or_loom::sync::atomic::AtomicI32 or the crate's
re-export) so loom sees these atomics, updating the use statement in
multiindexed/mod.rs and any other direct std::sync::atomic references; if
AtomicI32 is not yet re-exported, add or use the appropriate re-export in
crate::std_or_loom so the bounds/synchronization helpers are modeled by loom and
adjust any Ordering uses to come from the same compatibility module.
- Around line 256-285: The current is_empty() only checks dimension 0 and can
return false while other dimensions still hold sentinel values; change
is_empty() to iterate all dimensions and return true if any
self.min_coords[i].load(...) > self.max_coords[i].load(...) (i.e. treat the
structure as empty if any dimension is uninitialized), so min_coords() and
max_coords() never expose sentinel values; reference update_bounds(),
is_empty(), min_coords(), max_coords(), and the min_coords / max_coords arrays
when making this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d19cc091-0745-486d-9805-89c1e4f83826
📒 Files selected for processing (8)
ext/crates/once/src/grove/block.rsext/crates/once/src/multiindexed/iter.rsext/crates/once/src/multiindexed/kdtrie.rsext/crates/once/src/multiindexed/mod.rsext/crates/sseq/src/bigraded.rsext/crates/sseq/src/lib.rsext/crates/sseq/src/sseq.rsweb_ext/sseq_gui/src/sseq.rs
💤 Files with no reviewable changes (2)
- ext/crates/sseq/src/lib.rs
- ext/crates/sseq/src/bigraded.rs
ed75fce to
4b4c863
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
ext/crates/once/src/multiindexed/mod.rs (1)
268-294:⚠️ Potential issue | 🟠 Major
is_empty()still leaks the sentinel when the last coordinate isi32::MIN.Using Lines 273-274 as the publication guard still collides with a valid inserted value. If the first visible insert ends in
i32::MIN,is_empty()can flip tofalsewithout a distinguishable publish on that guard coordinate, andmin_coords()/max_coords()can still readi32::MAX/i32::MINfrom earlier dimensions. A separate initialized flag, or reverting to an all-dimension emptiness check, avoids that edge case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ext/crates/sseq/src/sseq.rs`:
- Around line 180-200: The code panics for omitted/zero-dimensional targets
because extend_differential blindly creates a Differential sized by
self.dimension(target) and extend_page_data clones the last page_data even when
none exists; modify extend_differential (function extend_differential) to check
the target dimension (let target = P::profile(r, b); let target_dim =
self.dimension(target)) and if target_dim == 0 create a Differential with zero
target dimension (or skip adding basis entries) instead of unwrapping or
assuming nonzero, and modify extend_page_data (function extend_page_data) to
handle an empty bd.page_data by pushing a default/zero PageData (construct an
empty page representation) when bd.page_data.is_empty() rather than calling
bd.page_data.last().unwrap().clone(), so extending works for omitted zero
bidegrees and higher-page differentials without panicking.
- Around line 97-109: The constructor Sseq::new currently ignores its _min
parameter so Sseq::min() returns (0,0) until data has entries; update the Sseq
struct to store the provided min (e.g., add a field like initial_min or
min_bidegree), set that field in Sseq::new(p: ValidPrime, min: Bidegree), and
modify Sseq::min() to return the stored min when self.data.min_coords() yields
None (otherwise keep returning Bidegree::x_y(min[0], min[1]) as before). Ensure
fields referenced are the existing ones (p, data, profile) and adjust any
constructors/usages accordingly so web_ext/sseq_gui’s calls to inner.min().y()
see the configured minimum before any insertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a06bb28b-0fb4-4d9c-af2a-51c6ed9ca157
📒 Files selected for processing (6)
ext/crates/once/src/multiindexed/iter.rsext/crates/once/src/multiindexed/mod.rsext/crates/sseq/src/bigraded.rsext/crates/sseq/src/lib.rsext/crates/sseq/src/sseq.rsweb_ext/sseq_gui/src/sseq.rs
💤 Files with no reviewable changes (2)
- ext/crates/sseq/src/lib.rs
- ext/crates/sseq/src/bigraded.rs
4b4c863 to
6728523
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
ext/crates/once/src/multiindexed/mod.rs (1)
261-265:⚠️ Potential issue | 🟠 MajorThe emptiness publication still aliases a valid coordinate at
i32::MIN.Line 273 fixed the old “check dimension 0” bug, but the new scheme still uses a real coordinate value as the published empty/non-empty boundary. On a first insert whose last coordinate is
i32::MIN,min_coords[K - 1]can already readi32::MINwhilemax_coords[K - 1]is still the initializeri32::MIN, sois_empty()returnsfalsewithout synchronizing with the insert. In that window,min_coords()/max_coords()can still leak sentinels from earlier dimensions. This needs a separate initialized flag or another non-aliasing publication mechanism.Also applies to: 269-275
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ext/crates/once/src/multiindexed/mod.rs` around lines 261 - 265, The published empty/non-empty boundary currently aliases real coordinate value i32::MIN causing is_empty() to misreport when a true coordinate equals the sentinel; update_bounds (and related code that reads min_coords, max_coords, min_coords(), max_coords(), and is_empty) should stop using a real coordinate as the sentinel and instead publish initialization state separately—introduce a per-dimension AtomicBool (or a single AtomicBool for the whole point if appropriate) that is set when a dimension has been initialized, update update_bounds to set that flag after storing min/max, and update is_empty/min_coords/max_coords readers to consult thatinitialized flag before trusting min/max values so no real coordinate value can be confused with the sentinel.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ext/crates/sseq/src/sseq.rs`:
- Around line 363-369: The current multiply implementation treats a missing
Product matrix as the zero map by creating a zero FpVector and proceeding;
instead, preserve the "undefined" contract by returning None when the product
entry is missing. Change multiply (in ext/crates/sseq/src/sseq.rs) to compute
target_b and call get_dimension(target_b)? first, then check
prod.matrices.get(elem.degree()) and if it is None return None immediately; only
when the matrix exists create the FpVector, call matrix.apply(...), and return
Some(BidegreeElement::new(...)). Ensure you reference the existing symbols
multiply, BidegreeElement, Product, prod.matrices.get, get_dimension, and
FpVector so the check and early return replace the current zero-vector behavior.
In `@web_ext/sseq_gui/src/sseq.rs`:
- Around line 490-500: The add_product replay path currently calls
Bigraded::insert on product.inner.matrices which panics if an entry for (name,
b) already exists; change this to have overwrite semantics: either use the
Bigraded API that replaces existing entries (e.g. an
insert_or_replace/replace/overwrite variant) or explicitly remove the existing
key from product.inner.matrices before inserting the new matrix (call remove(&b)
then insert). Also, if clear() is intended to discard old user matrices,
reinitialize or clear product.inner.matrices in the clear() implementation so
stale storage isn’t preserved for undo/redo.
---
Duplicate comments:
In `@ext/crates/once/src/multiindexed/mod.rs`:
- Around line 261-265: The published empty/non-empty boundary currently aliases
real coordinate value i32::MIN causing is_empty() to misreport when a true
coordinate equals the sentinel; update_bounds (and related code that reads
min_coords, max_coords, min_coords(), max_coords(), and is_empty) should stop
using a real coordinate as the sentinel and instead publish initialization state
separately—introduce a per-dimension AtomicBool (or a single AtomicBool for the
whole point if appropriate) that is set when a dimension has been initialized,
update update_bounds to set that flag after storing min/max, and update
is_empty/min_coords/max_coords readers to consult thatinitialized flag before
trusting min/max values so no real coordinate value can be confused with the
sentinel.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7c961713-b4c4-44ca-b978-b746d904b064
📒 Files selected for processing (6)
ext/crates/once/src/multiindexed/mod.rsext/crates/sseq/src/bigraded.rsext/crates/sseq/src/lib.rsext/crates/sseq/src/sseq.rsext/src/chain_complex/mod.rsweb_ext/sseq_gui/src/sseq.rs
6728523 to
7ffcd0a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ext/crates/sseq/src/bigraded.rs`:
- Around line 53-65: Add doc comments to the Index and IndexMut impls for
Bigraded<V> stating that index(&self, b: Bidegree) and index_mut(&mut self, b:
Bidegree) will panic if the bidegree is not present (they call
self.get(b).unwrap() / self.get_mut(b).unwrap()), and that callers must ensure
the bidegree is defined before indexing; reference the alternative get/get_mut
which return Option to make the difference explicit.
In `@ext/crates/sseq/src/sseq.rs`:
- Around line 140-147: Add clear doc comments to clarify panicking vs Option
semantics: update the doc for dimension(&self, b: Bidegree) to state it returns
the dimension for an existing bidegree and will panic if the bidegree is
undefined (so callers should ensure existence or use get_dimension), and update
the doc for get_dimension(&self, b: Bidegree) to state it returns Option<usize>
and should be used when the bidegree may be undefined (per the "undefined means
undefined" design contract). Reference the functions dimension and get_dimension
and mention the panic condition and preferred call sites.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 28395562-43e2-44d2-8ac6-365f61f6b6d7
📒 Files selected for processing (5)
ext/crates/sseq/src/bigraded.rsext/crates/sseq/src/lib.rsext/crates/sseq/src/sseq.rsext/src/chain_complex/mod.rsweb_ext/sseq_gui/src/sseq.rs
|
Note Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
|
❌ Failed to create PR with unit tests: AGENT_CHAT: Failed to open pull request |
7ffcd0a to
9a8d56d
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web_ext/sseq_gui/src/sseq.rs (1)
89-98:⚠️ Potential issue | 🔴 CriticalUndo or redo is no longer clearing sparse state safely.
clear()still preservesinner,class_names, andstale, but those are now presence-basedBigradeds. Old bidegrees therefore survive an undo, and replayingset_dimension()at Lines 300-311 hitsinserton the same keys again; if dimensions are not replayed, stale names and flags from the old history remain live. This needs a real reset for the sparse maps, orset_dimension()must reinitialize existing entries instead of inserting.🧹 Minimal safe reset
pub fn clear(&mut self) { for prod in self.products.values_mut() { if prod.user { prod.permanent = false; } prod.differential = None; } - self.inner.clear(); + self.inner = Sseq::new(self.p, Bidegree::zero()); + self.class_names = Bigraded::new(); + self.stale = Bigraded::new(); }Based on learnings: In
ext/crates/sseq/src/sseq.rs, the design contract forSseqis "undefined means undefined, do not assume anything" — a bidegree not present indata: MultiIndexed<2, BidegreeData>is simply not yet set; callers must not treat absence as zero-dimensional.Also applies to: 300-311
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web_ext/sseq_gui/src/sseq.rs` around lines 89 - 98, The clear() method currently leaves presence-based Bigraded sparse maps intact (self.inner, self.class_names, self.stale), which preserves old bidegree keys across undo/redo; update clear() to fully reset those sparse maps (e.g., call clear() or reinitialize the MultiIndexed/Biggraded structures for self.inner, self.class_names, and self.stale) so no old bidegrees remain, or alternately change set_dimension() to reinitialize existing entries instead of calling insert (use entry API or replace existing BidegreeData) — locate clear(), the fields products, inner, class_names, stale, and the set_dimension() implementation (the insert calls at the set_dimension() block) and make one of these two consistent fixes.
♻️ Duplicate comments (1)
ext/crates/once/src/multiindexed/mod.rs (1)
285-291:⚠️ Potential issue | 🟠 Major
is_empty()leaks partially initialized state when first inserted point hascoords[K - 1] == i32::MIN.With sentinels
min_coords = i32::MAXandmax_coords = i32::MIN, the check at line 290 becomesi32::MIN > i32::MIN = falsewhen the first insert has that coordinate value, falsely reporting the array as non-empty without synchronizing with all dimension updates. Earlier dimensions remain uninitialized sentinels but are returned bymin_coords()andmax_coords()after trusting the falseis_empty()check. Check all K dimensions to detect the sentinel pattern in any dimension:Suggested fix
pub fn is_empty(&self) -> bool { - // We check the last coordinate because it is the one that is written to last when we - // insert. Observing a consistent last coordinate means that all coordinates are consistent, - // and calling `min_coords` or `max_coords` will not return nonsense. - self.min_coords[K - 1].load(Ordering::Acquire) - > self.max_coords[K - 1].load(Ordering::Acquire) + (0..K).any(|i| { + self.min_coords[i].load(Ordering::Acquire) + > self.max_coords[i].load(Ordering::Acquire) + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ext/crates/once/src/multiindexed/mod.rs` around lines 285 - 291, is_empty currently only checks the last dimension and can misreport non-empty when a newly inserted value equals i32::MIN in that last coordinate; change is_empty to iterate all K dimensions and perform Acquire loads from min_coords[i] and max_coords[i], returning true only if every dimension satisfies min > max (the sentinel empty condition), otherwise return false as soon as any dimension shows min <= max; update references to min_coords, max_coords, K, and the Ordering::Acquire loads accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@web_ext/sseq_gui/src/sseq.rs`:
- Around line 89-98: The clear() method currently leaves presence-based Bigraded
sparse maps intact (self.inner, self.class_names, self.stale), which preserves
old bidegree keys across undo/redo; update clear() to fully reset those sparse
maps (e.g., call clear() or reinitialize the MultiIndexed/Biggraded structures
for self.inner, self.class_names, and self.stale) so no old bidegrees remain, or
alternately change set_dimension() to reinitialize existing entries instead of
calling insert (use entry API or replace existing BidegreeData) — locate
clear(), the fields products, inner, class_names, stale, and the set_dimension()
implementation (the insert calls at the set_dimension() block) and make one of
these two consistent fixes.
---
Duplicate comments:
In `@ext/crates/once/src/multiindexed/mod.rs`:
- Around line 285-291: is_empty currently only checks the last dimension and can
misreport non-empty when a newly inserted value equals i32::MIN in that last
coordinate; change is_empty to iterate all K dimensions and perform Acquire
loads from min_coords[i] and max_coords[i], returning true only if every
dimension satisfies min > max (the sentinel empty condition), otherwise return
false as soon as any dimension shows min <= max; update references to
min_coords, max_coords, K, and the Ordering::Acquire loads accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ec00182b-ddbb-4903-bc7b-b358f8952968
📒 Files selected for processing (7)
ext/crates/once/src/multiindexed/iter.rsext/crates/once/src/multiindexed/mod.rsext/crates/sseq/src/bigraded.rsext/crates/sseq/src/lib.rsext/crates/sseq/src/sseq.rsext/src/chain_complex/mod.rsweb_ext/sseq_gui/src/sseq.rs
Replace .get_mut(b).unwrap() and .get(b).unwrap() calls on Bigraded fields (stale, class_names) with index operator syntax. The Index and IndexMut impls already provide a panic message with the bidegree on missing entries. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
9a8d56d to
6a27390
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web_ext/sseq_gui/src/sseq.rs (1)
53-69: 🛠️ Refactor suggestion | 🟠 MajorMake
innerprivate before the sparse side tables can drift out of sync.
staleandclass_namesnow only get entries throughSseqWrapper::set_dimension. Keepinginnerpublicly mutable lets outside code insert bidegrees throughinner.set_dimension(...)without creating those companion entries, and the nextrefresh,send_class_data, orset_class_namecan panic onself.stale[b]/self.class_names[b].🔒 Minimal API tightening
pub struct SseqWrapper<P: SseqProfile = Adams> { pub p: ValidPrime, name: SseqChoice, - pub inner: Sseq<P>, + inner: Sseq<P>,impl<P: SseqProfile> SseqWrapper<P> { pub fn inner(&self) -> &Sseq<P> { &self.inner } }Based on learnings: in
ext/crates/sseq/src/sseq.rs, missing bidegrees are undefined, not zero-dimensional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web_ext/sseq_gui/src/sseq.rs` around lines 53 - 69, SseqWrapper exposes its inner Sseq via the pub field `inner`, letting external code call `inner.set_dimension(...)` and create bidegrees without updating `stale`/`class_names`, causing panics; make the `inner` field private (remove `pub` on `inner`) and add a public accessor `fn inner(&self) -> &Sseq<P>` so callers can read but not mutate the inner Sseq; update any callsites that relied on mutating `inner` to go through SseqWrapper methods (e.g. `set_dimension`) so `stale` and `class_names` are kept in sync.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@web_ext/sseq_gui/src/sseq.rs`:
- Around line 53-69: SseqWrapper exposes its inner Sseq via the pub field
`inner`, letting external code call `inner.set_dimension(...)` and create
bidegrees without updating `stale`/`class_names`, causing panics; make the
`inner` field private (remove `pub` on `inner`) and add a public accessor `fn
inner(&self) -> &Sseq<P>` so callers can read but not mutate the inner Sseq;
update any callsites that relied on mutating `inner` to go through SseqWrapper
methods (e.g. `set_dimension`) so `stale` and `class_names` are kept in sync.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 829d9a61-2db7-482c-8475-3f7d22979055
📒 Files selected for processing (6)
ext/crates/sseq/src/bigraded.rsext/crates/sseq/src/lib.rsext/crates/sseq/src/sseq.rsext/src/chain_complex/mod.rsweb_ext/sseq_gui/src/managers.rsweb_ext/sseq_gui/src/sseq.rs
* Change Sseq internals * Remove `DenseBigradedModule` * Add `Sseq::iter_bidegrees` * More migration * Introduce `Bigraded` thin wrapper * Fix Sseq::multiply * Add error messages to `Index` * Remove useless `min` parameter * Use Bigraded indexing instead of get/get_mut().unwrap() Replace .get_mut(b).unwrap() and .get(b).unwrap() calls on Bigraded fields (stale, class_names) with index operator syntax. The Index and IndexMut impls already provide a panic message with the bidegree on missing entries.
* Change Sseq internals * Remove `DenseBigradedModule` * Add `Sseq::iter_bidegrees` * More migration * Introduce `Bigraded` thin wrapper * Fix Sseq::multiply * Add error messages to `Index` * Remove useless `min` parameter * Use Bigraded indexing instead of get/get_mut().unwrap() Replace .get_mut(b).unwrap() and .get(b).unwrap() calls on Bigraded fields (stale, class_names) with index operator syntax. The Index and IndexMut impls already provide a panic message with the bidegree on missing entries.
1 participant
Summary by CodeRabbit
Release Notes