Unify vector-like structs #214
Conversation
The similarly-named but crucially different `FqSlice::slice` has been renamed to `restrict`
This solves the compilation issues when odd-primes is disabled
📝 WalkthroughWalkthroughThis pull request refactors the vector and matrix type systems by introducing generic base types ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ext/crates/fp/src/vector/impl_fqslicemut.rs (1)
38-41: Consider usingself.len()directly instead ofself.as_slice().len().Since
FqSliceMuthas alen()method through the base type,self.as_slice().len()creates an unnecessary intermediateFqSlice. This is a minor inefficiency but inconsistent with similar patterns elsewhere in the file (e.g., line 412).🔎 Proposed fix
pub fn add_offset(&mut self, other: FqSlice<'_, F>, c: FieldElement<F>, offset: usize) { - self.slice_mut(offset, self.as_slice().len()) + self.slice_mut(offset, self.len()) .add(other.slice(offset, other.len()), c) }
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
ext/crates/algebra/src/module/free_module.rsext/crates/fp/src/matrix/matrix_inner.rsext/crates/fp/src/vector/fp_wrapper/helpers.rsext/crates/fp/src/vector/fp_wrapper/mod.rsext/crates/fp/src/vector/impl_fqslice.rsext/crates/fp/src/vector/impl_fqslicemut.rsext/crates/fp/src/vector/impl_fqvector.rsext/crates/fp/src/vector/inner.rs
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-12-11T08:35:31.031Z
Learnt from: JoeyBF
Repo: SpectralSequences/sseq PR: 205
File: ext/crates/fp/src/limb.rs:11-28
Timestamp: 2025-12-11T08:35:31.031Z
Learning: In Rust 2024 edition (Cargo.toml edition = 2024), the mem::size_of, size_of_val, align_of, and align_of_val functions from std::mem are in the prelude and do not require explicit imports. During reviews, these can be used unqualified when the project targets edition = 2024. Verify the crate's Cargo.toml edition setting before assuming unqualified use, and avoid adding extraneous imports for these helpers.
Applied to files:
ext/crates/algebra/src/module/free_module.rsext/crates/fp/src/vector/impl_fqslicemut.rsext/crates/fp/src/vector/impl_fqslice.rsext/crates/fp/src/vector/impl_fqvector.rsext/crates/fp/src/vector/fp_wrapper/helpers.rsext/crates/fp/src/vector/inner.rsext/crates/fp/src/matrix/matrix_inner.rsext/crates/fp/src/vector/fp_wrapper/mod.rs
📚 Learning: 2025-12-14T08:23:53.399Z
Learnt from: JoeyBF
Repo: SpectralSequences/sseq PR: 207
File: ext/crates/fp/src/simd/x86_64/avx512.rs:367-379
Timestamp: 2025-12-14T08:23:53.399Z
Learning: In Rust 2024 edition, do not remove explicit unsafe blocks inside unsafe fn bodies. The lint unsafe_op_in_unsafe_fn is enabled by default, treating the body of an unsafe fn as safe and requiring each unsafe operation to be wrapped in an explicit unsafe { ... } block. Do not suggest removing these blocks in code using edition = "2024"; ensure unsafe operations are enclosed in explicit unsafe blocks and review for unnecessary nesting.
Applied to files:
ext/crates/algebra/src/module/free_module.rsext/crates/fp/src/vector/impl_fqslicemut.rsext/crates/fp/src/vector/impl_fqslice.rsext/crates/fp/src/vector/impl_fqvector.rsext/crates/fp/src/vector/fp_wrapper/helpers.rsext/crates/fp/src/vector/inner.rsext/crates/fp/src/matrix/matrix_inner.rsext/crates/fp/src/vector/fp_wrapper/mod.rs
📚 Learning: 2025-12-15T22:15:17.779Z
Learnt from: JoeyBF
Repo: SpectralSequences/sseq PR: 207
File: ext/crates/fp/benches/mul.rs:19-28
Timestamp: 2025-12-15T22:15:17.779Z
Learning: In the fp crate, matrices over F_2 (prime=2) are stored with 1 bit per matrix entry. This means an M×N matrix uses M*N bits, i.e. M*N/8 bytes. For example, an 8192×8192 F_2 matrix uses (8192*8192)/8 = 8,388,608 bytes = 8 MB. Use this packing assumption when analyzing memory usage and bench tests, and rely on the crate's API for bit-packed access rather than byte-per-entry storage.
Applied to files:
ext/crates/fp/src/vector/impl_fqslicemut.rsext/crates/fp/src/vector/impl_fqslice.rsext/crates/fp/src/vector/impl_fqvector.rsext/crates/fp/src/vector/fp_wrapper/helpers.rsext/crates/fp/src/vector/inner.rsext/crates/fp/src/matrix/matrix_inner.rsext/crates/fp/src/vector/fp_wrapper/mod.rs
📚 Learning: 2025-12-20T04:55:22.617Z
Learnt from: JoeyBF
Repo: SpectralSequences/sseq PR: 207
File: ext/crates/fp/src/matrix/matrix_inner.rs:75-89
Timestamp: 2025-12-20T04:55:22.617Z
Learning: In reviews of code under ext/crates/fp/, treat AVec::from_iter(0, data) as using AVec's default alignment, not 0-byte alignment. Do not assume a reduced alignment or override of AVec's inherent alignment guarantees. If code relies on a specific alignment, verify it via AVec's type alias and its documentation, and avoid making changes that could alter alignment semantics. Ensure tests or comments reflect that 0 is a default alignment flag and not a zero-byte alignment.
Applied to files:
ext/crates/fp/src/vector/impl_fqslicemut.rsext/crates/fp/src/vector/impl_fqslice.rsext/crates/fp/src/vector/impl_fqvector.rsext/crates/fp/src/vector/fp_wrapper/helpers.rsext/crates/fp/src/vector/inner.rsext/crates/fp/src/matrix/matrix_inner.rsext/crates/fp/src/vector/fp_wrapper/mod.rs
🧬 Code graph analysis (5)
ext/crates/fp/src/vector/impl_fqslicemut.rs (3)
ext/crates/fp/src/vector/inner.rs (4)
fq(68-70)len(76-78)start(150-152)end(154-156)ext/crates/fp/src/vector/iter.rs (1)
len(110-112)ext/crates/fp/src/vector/impl_fqslice.rs (1)
from(42-44)
ext/crates/fp/src/vector/impl_fqslice.rs (3)
ext/crates/fp/src/vector/inner.rs (4)
start(150-152)end(154-156)len(76-78)_new(54-66)ext/crates/fp/src/vector/fp_wrapper/mod.rs (3)
from(194-196)from(253-255)from(259-261)ext/crates/fp/src/vector/impl_fqslicemut.rs (1)
from(445-447)
ext/crates/fp/src/vector/inner.rs (5)
ext/crates/fp/src/matrix/matrix_inner.rs (8)
iter(380-382)iter(1412-1419)prime(166-168)prime(1388-1390)new(47-49)new(1209-1224)fmt(430-445)fmt(449-451)ext/crates/fp/src/field/element.rs (3)
field(45-47)new(41-43)fmt(110-112)ext/crates/algebra/src/algebra/algebra_trait.rs (1)
prime(42-42)ext/crates/fp/src/vector/impl_fqslicemut.rs (2)
new(168-202)new(288-321)ext/crates/fp/src/vector/fp_wrapper/mod.rs (1)
fmt(180-190)
ext/crates/fp/src/matrix/matrix_inner.rs (1)
ext/crates/fp/src/vector/inner.rs (4)
_new(54-66)limb_range(172-174)start(150-152)end(154-156)
ext/crates/fp/src/vector/fp_wrapper/mod.rs (2)
ext/crates/fp/src/vector/inner.rs (8)
iter(101-103)len(76-78)slice(85-94)as_slice(96-98)iter_nonzero(105-107)limbs(158-160)slice_mut(311-321)as_slice_mut(215-217)ext/crates/fp/src/vector/impl_fqslicemut.rs (1)
from(445-447)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: webserver (beta)
- GitHub Check: test (stable)
- GitHub Check: test (beta)
- GitHub Check: webserver (nightly)
- GitHub Check: miri
- GitHub Check: lint
- GitHub Check: test (nightly)
🔇 Additional comments (16)
ext/crates/fp/src/matrix/matrix_inner.rs (2)
368-376: LGTM: Row accessor constructor updates are consistent.The switch from
FpSlice::new/FpSliceMut::newto_newaligns with the new internal constructor visibility pattern introduced in the vector base refactor. The parameters and semantics remain unchanged.
1266-1272: Appropriate use of new slicing APIs.Line 1266 correctly uses the internal
_newconstructor for mutable slice construction, while line 1272 uses the new publicrestrict()method for sub-slicing an immutable slice. This follows the intended API split between internal construction and public sub-slicing.ext/crates/fp/src/vector/fp_wrapper/helpers.rs (2)
23-41: Clean separation of read vs. write helpers using Repr traits.The split of helpers into
Repr(read-only:entry_helper) andReprMut(mutable:scale_helper,set_entry_helper,add_basis_element_helper) impl blocks correctly aligns with the trait bounds and follows the new representation abstraction pattern.
72-79: Newadd_offset_helperis consistent with existing helper pattern.The helper follows the established pattern of converting
F::ElementContainer(u32) toFieldElement<F>before delegating to the underlying method.ext/crates/fp/src/vector/impl_fqslicemut.rs (1)
442-447: Generic From implementation correctly uses ReprMut bound.The generalization from
From<&'a mut FqVector<F>>toFrom<&'a mut FqVectorBase<A, R, F>>withR: ReprMutallows conversion from any mutable vector-base representation, aligning with the unified type design.ext/crates/fp/src/vector/impl_fqvector.rs (2)
5-5: Import path updated to reflect new type alias location.
FqVector<F>is now a type alias toFqVectorBase<true, Vec<Limb>, F>defined ininner.rs, so the import correctly points there.
11-245: FqVector implementation correctly retains owned-vector-specific operations.Methods that require ownership or Vec-specific operations (construction, resize, capacity management, limb-level arithmetic) are appropriately kept here, while common operations have been moved to
FqVectorBase.ext/crates/fp/src/vector/impl_fqslice.rs (2)
11-21: Appropriate rename fromslice()torestrict().The rename to
restrict()clarifies that this method narrows an existing slice rather than creating a slice from an owned vector, avoiding confusion withFqVectorBase::slice(). The implementation correctly uses the internal_newconstructor and preserves the lifetime throughinto_limbs().
41-44: Generic From implementation correctly uses Repr bound.The generalization allows converting any
FqVectorBasewith aRepr-bounded storage type to anFqSlice, enabling uniform slice creation from vectors, slices, and other representations.ext/crates/fp/src/vector/fp_wrapper/mod.rs (3)
48-63: Well-designed enum-based wrapper with generic representation.The
FpVectorBase<const A: bool, R>enum cleanly abstracts over prime variants while theAandRparameters enable the same aligned/representation flexibility as the underlyingFqVectorBase. The type aliases provide ergonomic access to the most common configurations.
73-100: Correct trait-bounded impl blocks for read vs. write operations.The separation into
Repr(read:slice,as_slice,iter,iter_nonzero,is_zero,entry,limbs) andReprMut(write:slice_mut,as_slice_mut,set_to_zero,scale,set_entry,add_basis_element,limbs_mut) impl blocks correctly mirrors the underlyingFqVectorBasedesign.
142-148: FpSlice API aligns with FqSlice changes.The
_newconstructor (pub(crate)),restrict(replacingslice), andto_ownedmethods match the correspondingFqSliceAPI, maintaining consistency across the Fp/Fq abstraction layers.ext/crates/fp/src/vector/inner.rs (4)
18-24: Clean trait design for storage representation abstraction.The
ReprandReprMuttraits with blanket implementations overDeref<Target = [Limb]>andDerefMut<Target = [Limb]>respectively enable flexible storage strategies (Vec, slices, Cow, Arc, etc.) while maintaining a uniform interface.
53-65: Alignment validation in constructor is correct.The assertion at line 57 ensures that when
Ais true (aligned vectors), the start position is always on a limb boundary. This invariant enables the fast paths in methods likeis_zero()andset_to_zero().
176-184: Edge case documentation is appreciated.The comment explaining the potential underflow for zero-width slices at the start of an
FpVectoris helpful. The performance trade-off (avoidingsaturating_subfor a 10% gain) is reasonable given the rarity of this edge case.
339-363: Display implementation handles both formats correctly.The alternate format (
{:#}) provides compact representation suitable for prime fields < 10, while the default format provides a bracketed, comma-separated list. The implementation is consistent with theFpVectorBaseDisplay impl infp_wrapper/mod.rs.
| v.restrict( | ||
| std::cmp::min(v.len(), start), | ||
| std::cmp::min(v.len(), start + len), | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find all uses of .slice( in the codebase
rg -n '\.slice\(' --type=rust | head -50Repository: SpectralSequences/sseq
Length of output: 4511
🏁 Script executed:
# Check the specific file around lines 198 and 367
cat -n ext/crates/algebra/src/module/free_module.rs | sed -n '190,210p'
cat -n ext/crates/algebra/src/module/free_module.rs | sed -n '360,375p'Repository: SpectralSequences/sseq
Length of output: 1617
🏁 Script executed:
# Find FpSlice definition to check available methods
rg -n 'impl.*FpSlice' --type=rust -A 10 | head -100Repository: SpectralSequences/sseq
Length of output: 7329
🏁 Script executed:
# Search for the restrict method definition
rg -n 'fn restrict' --type=rust -B 2 -A 5Repository: SpectralSequences/sseq
Length of output: 1864
🏁 Script executed:
# Search for the definition of .slice( method
rg -n 'fn slice' --type=rust -B 2 -A 5 ext/crates/fp/Repository: SpectralSequences/sseq
Length of output: 3272
🏁 Script executed:
# Check the full impl blocks for FpSlice to see all methods
rg -n 'impl.*FpSlice|impl.*FqSlice' --type=rust -A 20 ext/crates/fp/src/vector/Repository: SpectralSequences/sseq
Length of output: 11424
Update line 198 to use the new .restrict() API.
Line 198 should use input.restrict(input_start, input_end) to be consistent with line 367. The FpSlice::slice() method does not exist; the new unified API is FpSlice::restrict().
🤖 Prompt for AI Agents
ext/crates/algebra/src/module/free_module.rs around lines 367-370 (also update
the use at line ~198): change the old FpSlice::slice(...) call to use the new
FpSlice::restrict(start, end) API; compute start as std::cmp::min(input.len(),
input_start) and end as std::cmp::min(input.len(), input_start + len) and pass
those to input.restrict(start, end) so it mirrors the usage at lines 367–370.
There was a problem hiding this comment.
Is it not already using restrict?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the exact line number where input.slice is used in this file
grep -n 'input.slice' ext/crates/algebra/src/module/free_module.rs
# Also show context around that line
grep -n -C 3 'input.slice' ext/crates/algebra/src/module/free_module.rs
Length of output: 631
@JoeyBF, you're right that line 367 (shown in the diff) is already using restrict() — that change looks good!
However, line 198 still uses the old .slice() method:
let input_slice = input.slice(input_start, input_end);This should be updated to use .restrict() as well, to match the new unified API used at line 367:
let input_slice = input.restrict(input_start, input_end);| pub fn scale(&mut self, c: FieldElement<F>) { | ||
| assert_eq!(self.fq(), c.field()); | ||
| let fq = self.fq(); | ||
|
|
||
| if c == fq.zero() { | ||
| self.set_to_zero(); | ||
| } | ||
|
|
||
| if fq.q() == 2 { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Missing return after set_to_zero() causes redundant operations.
When c == fq.zero(), the method calls set_to_zero() but then continues to execute the rest of the function. For q == 2, this results in an early return (correct). For other primes, it continues to scale the already-zeroed limbs, which is wasteful but not incorrect.
🔎 Proposed fix
if c == fq.zero() {
self.set_to_zero();
+ return;
}
if fq.q() == 2 {
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub fn scale(&mut self, c: FieldElement<F>) { | |
| assert_eq!(self.fq(), c.field()); | |
| let fq = self.fq(); | |
| if c == fq.zero() { | |
| self.set_to_zero(); | |
| } | |
| if fq.q() == 2 { | |
| return; | |
| } | |
| pub fn scale(&mut self, c: FieldElement<F>) { | |
| assert_eq!(self.fq(), c.field()); | |
| let fq = self.fq(); | |
| if c == fq.zero() { | |
| self.set_to_zero(); | |
| return; | |
| } | |
| if fq.q() == 2 { | |
| return; | |
| } |
🤖 Prompt for AI Agents
In ext/crates/fp/src/vector/inner.rs around lines 242 to 252, when c ==
fq.zero() the method calls self.set_to_zero() but does not return, causing the
function to continue and perform redundant scaling; update the control flow so
that after calling self.set_to_zero() the function returns immediately (i.e.,
add a return right after set_to_zero()) so the subsequent logic (including the
existing fq.q() == 2 check) is skipped for the zero scalar.
1 participant
This PR abstracts away the common logic behind
FqVector,FqSliceandFqSliceMut. This leads to simplifications and the possibility of adding more variants in the future.Currently, my main objective is switching to using
FpCowinBidegreeElements to avoid unnecessary clones and make them usable in performance-critical code.Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.