diff --git a/ext/crates/algebra/src/algebra/adem_algebra.rs b/ext/crates/algebra/src/algebra/adem_algebra.rs index af4678030d..100f42a506 100644 --- a/ext/crates/algebra/src/algebra/adem_algebra.rs +++ b/ext/crates/algebra/src/algebra/adem_algebra.rs @@ -326,7 +326,7 @@ impl GeneratedAlgebra for AdemAlgebra { fn generators(&self, degree: i32) -> Vec { let p = self.prime(); - if degree == 0 { + if degree <= 0 { return vec![]; } if self.generic { diff --git a/ext/crates/algebra/src/algebra/milnor_algebra.rs b/ext/crates/algebra/src/algebra/milnor_algebra.rs index 6ef65e58c4..4f0d0ac5e9 100644 --- a/ext/crates/algebra/src/algebra/milnor_algebra.rs +++ b/ext/crates/algebra/src/algebra/milnor_algebra.rs @@ -695,7 +695,7 @@ impl GeneratedAlgebra for MilnorAlgebra { } fn generators(&self, degree: i32) -> Vec { - if degree == 0 { + if degree <= 0 { return vec![]; } else if degree == 1 { return vec![0]; // Q_0 diff --git a/ext/crates/algebra/src/module/finite_dimensional_module.rs b/ext/crates/algebra/src/module/finite_dimensional_module.rs index 37d2640e77..58fed33b6f 100644 --- a/ext/crates/algebra/src/module/finite_dimensional_module.rs +++ b/ext/crates/algebra/src/module/finite_dimensional_module.rs @@ -546,7 +546,9 @@ impl FiniteDimensionalModule { input_deg: i32, output_deg: i32, ) -> Result<(), ModuleFailedRelationError> { - assert!(output_deg > input_deg); + if output_deg <= input_deg { + return Ok(()); + } let p = self.prime(); let algebra = self.algebra(); let op_deg = output_deg - input_deg; @@ -598,49 +600,50 @@ impl FiniteDimensionalModule { pub fn extend_actions(&mut self, input_deg: i32, output_deg: i32) { let p = self.prime(); let algebra = self.algebra(); - let op_deg = output_deg - input_deg; if self.dimension(output_deg) == 0 || self.dimension(input_deg) == 0 { return; } - let mut tmp_output = FpVector::new(p, self.dimension(output_deg)); + let op_deg = output_deg - input_deg; let generators = algebra.generators(op_deg); + let mut tmp_output = FpVector::new(p, self.dimension(output_deg)); for idx in 0..self.dimension(input_deg) { for op_idx in 0..algebra.dimension(op_deg) { - if !generators.contains(&op_idx) { - let mut output_vec = std::mem::replace( - &mut self.actions[input_deg][output_deg][op_idx][idx], - FpVector::new(p, 0), - ); - let decomposition = algebra.decompose_basis_element(op_deg, op_idx); - for (coef, (deg_1, idx_1), (deg_2, idx_2)) in decomposition { - let intermediate_dim = self.dimension(input_deg + deg_2); - if intermediate_dim > tmp_output.len() { - tmp_output = FpVector::new(p, intermediate_dim); - } - self.act_on_basis( - tmp_output.slice_mut(0, intermediate_dim), - 1, - deg_2, - idx_2, - input_deg, - idx, - ); - self.act( - output_vec.as_slice_mut(), - coef, - deg_1, - idx_1, - deg_2 + input_deg, - tmp_output.slice(0, intermediate_dim), - ); - tmp_output.set_to_zero(); + if generators.contains(&op_idx) { + continue; + } + let mut output_vec = std::mem::replace( + &mut self.actions[input_deg][output_deg][op_idx][idx], + FpVector::new(p, 0), + ); + let decomposition = algebra.decompose_basis_element(op_deg, op_idx); + for (coef, (deg_1, idx_1), (deg_2, idx_2)) in decomposition { + let intermediate_dim = self.dimension(input_deg + deg_2); + if intermediate_dim > tmp_output.len() { + tmp_output = FpVector::new(p, intermediate_dim); } - let _ = std::mem::replace( - &mut self.actions[input_deg][output_deg][op_idx][idx], - output_vec, + self.act_on_basis( + tmp_output.slice_mut(0, intermediate_dim), + 1, + deg_2, + idx_2, + input_deg, + idx, + ); + self.act( + output_vec.as_slice_mut(), + coef, + deg_1, + idx_1, + deg_2 + input_deg, + tmp_output.slice(0, intermediate_dim), ); + tmp_output.set_to_zero(); } + let _ = std::mem::replace( + &mut self.actions[input_deg][output_deg][op_idx][idx], + output_vec, + ); } } } @@ -803,4 +806,24 @@ mod tests { Err(ActError::InvalidInput(_)) )); } + + #[test] + fn extend_actions_negative_op_degree_is_safe() { + let mut module = make_test_module(); + // output < input means a negative op degree; algebra.generators(negative) + // returns an empty set, so this is a safe no-op rather than an + // out-of-bounds panic. + module.extend_actions(1, 0); + module.extend_actions(2, 0); + module.extend_actions(2, 1); + } + + #[test] + fn check_validity_non_increasing_bidegree_is_ok() { + let module = make_test_module(); + assert!(module.check_validity(0, 1).is_ok()); + // A non-increasing bidegree is a no-op returning Ok rather than asserting. + assert!(module.check_validity(1, 1).is_ok()); + assert!(module.check_validity(2, 1).is_ok()); + } } diff --git a/ext/crates/algebra/src/module/hom_module.rs b/ext/crates/algebra/src/module/hom_module.rs index 4035951d5e..e192a92143 100644 --- a/ext/crates/algebra/src/module/hom_module.rs +++ b/ext/crates/algebra/src/module/hom_module.rs @@ -28,19 +28,28 @@ impl std::fmt::Display for HomModule { } impl HomModule { - pub fn new(source: Arc>, target: Arc) -> Self { + /// Fallible version of [`new`](Self::new). + /// + /// Returns `Err` when `target` is not bounded above (`target.max_degree()` + /// is `None`), which `HomModule` requires in order to be bounded below. + /// [`new`](Self::new) is simply `Self::try_new(source, target).unwrap()`. + pub fn try_new(source: Arc>, target: Arc) -> anyhow::Result { let p = source.prime(); let algebra = Arc::new(Field::new(p)); - let min_degree = source.min_degree() - - target - .max_degree() - .expect("HomModule requires target to be bounded"); - Self { + let max_degree = target.max_degree().ok_or_else(|| { + anyhow::anyhow!("HomModule requires the target module to be bounded above") + })?; + let min_degree = source.min_degree() - max_degree; + Ok(Self { algebra, source, target, block_structures: OnceBiVec::new(min_degree), // fn_degree -> blocks - } + }) + } + + pub fn new(source: Arc>, target: Arc) -> Self { + Self::try_new(source, target).unwrap() } pub fn source(&self) -> Arc> { @@ -153,4 +162,29 @@ mod tests { assert_eq!(hom.dimension(deg), target_dim); } } + + #[test] + fn test_try_new_bounded_target() { + let algebra = Arc::new(MilnorAlgebra::new(fp::prime::TWO, false)); + let f = Arc::new(FreeModule::new(Arc::clone(&algebra), "F0".to_string(), 0)); + let m = Arc::new( + FDModule::from_json(Arc::clone(&algebra), &crate::tests::joker_json()).unwrap(), + ); + + // A bounded target (the FDModule) succeeds. + let hom = HomModule::try_new(Arc::clone(&f), m).unwrap(); + assert_eq!(hom.min_degree(), -4); + } + + #[test] + fn test_try_new_unbounded_target_errors() { + let algebra = Arc::new(MilnorAlgebra::new(fp::prime::TWO, false)); + let f = Arc::new(FreeModule::new(Arc::clone(&algebra), "F0".to_string(), 0)); + // A FreeModule is unbounded above (`max_degree()` is `None`), so it is not + // a valid Hom target: `try_new` errors instead of the `expect` panic in `new`. + let unbounded = Arc::new(FreeModule::new(Arc::clone(&algebra), "T".to_string(), 0)); + let result = HomModule::try_new(f, unbounded); + assert!(result.is_err()); + assert!(result.err().unwrap().to_string().contains("bounded above")); + } } diff --git a/ext/crates/algebra/src/module/homomorphism/hom_pullback.rs b/ext/crates/algebra/src/module/homomorphism/hom_pullback.rs index 4b04a8440c..197050fb24 100644 --- a/ext/crates/algebra/src/module/homomorphism/hom_pullback.rs +++ b/ext/crates/algebra/src/module/homomorphism/hom_pullback.rs @@ -23,24 +23,46 @@ pub struct HomPullback { } impl HomPullback { - pub fn new( + /// Fallible version of [`new`](Self::new). + /// + /// Returns `Err` unless `source`, `target` and `map` are wired together + /// consistently: `source = Hom(B, X)`, `target = Hom(A, X)` and `map: A -> + /// B`. + pub fn try_new( source: Arc>, target: Arc>, map: Arc>>, - ) -> Self { - assert!(Arc::ptr_eq(&source.source(), &map.target())); - assert!(Arc::ptr_eq(&target.source(), &map.source())); - assert!(Arc::ptr_eq(&source.target(), &target.target())); + ) -> anyhow::Result { + anyhow::ensure!( + Arc::ptr_eq(&source.source(), &map.target()), + "HomPullback: source.source() must be the map's target module" + ); + anyhow::ensure!( + Arc::ptr_eq(&target.source(), &map.source()), + "HomPullback: target.source() must be the map's source module" + ); + anyhow::ensure!( + Arc::ptr_eq(&source.target(), &target.target()), + "HomPullback: source and target must have the same Hom target module" + ); let min_degree = source.min_degree(); - Self { + Ok(Self { source, target, map, images: OnceBiVec::new(min_degree), kernels: OnceBiVec::new(min_degree), quasi_inverses: OnceBiVec::new(min_degree), - } + }) + } + + pub fn new( + source: Arc>, + target: Arc>, + map: Arc>>, + ) -> Self { + Self::try_new(source, target, map).unwrap() } } @@ -262,4 +284,54 @@ mod tests { assert_eq!(matrix, outputs[deg]); } } + + #[test] + fn try_new_wiring() { + const SHIFT: i32 = 2; + const NUM_GENS: [usize; 3] = [1, 2, 1]; + + let p = fp::prime::TWO; + + let algebra = Arc::new(MilnorAlgebra::new(p, false)); + let f0 = Arc::new(FreeModule::new(Arc::clone(&algebra), "F0".to_string(), 0)); + let f1 = Arc::new(FreeModule::new( + Arc::clone(&algebra), + "F1".to_string(), + SHIFT, + )); + let m = Arc::new( + FDModule::from_json(Arc::clone(&algebra), &crate::tests::joker_json()).unwrap(), + ); + + let d = Arc::new(FreeModuleHomomorphism::new( + Arc::clone(&f1), + Arc::clone(&f0), + SHIFT, + )); + + f0.compute_basis(NUM_GENS.len() as i32); + f1.compute_basis(NUM_GENS.len() as i32 + SHIFT); + for (deg, num_gens) in NUM_GENS.into_iter().enumerate() { + f0.add_generators(deg as i32, num_gens, None); + f1.add_generators(deg as i32 + SHIFT, num_gens, None); + let mut rows = vec![FpVector::new(p, f0.dimension(deg as i32)); num_gens]; + for (i, row) in rows.iter_mut().enumerate() { + row.add_basis_element(row.len() - num_gens + i, 1); + } + d.add_generators_from_rows(deg as i32 + SHIFT, rows); + } + + let source = Arc::new(HomModule::new(Arc::clone(&f0), Arc::clone(&m))); + let target = Arc::new(HomModule::new(Arc::clone(&f1), Arc::clone(&m))); + + // Correctly wired: source = Hom(f0, m), target = Hom(f1, m), d: f1 -> f0. + assert!( + HomPullback::try_new(Arc::clone(&source), Arc::clone(&target), Arc::clone(&d)).is_ok() + ); + + // Swapping source and target breaks the pointer-identity wiring: report + // an error rather than the `assert!` panic in `new`. + let result = HomPullback::try_new(target, source, d); + assert!(result.is_err()); + } } diff --git a/ext/crates/algebra/src/module/quotient_module.rs b/ext/crates/algebra/src/module/quotient_module.rs index f0b7c26d60..ba983bce69 100644 --- a/ext/crates/algebra/src/module/quotient_module.rs +++ b/ext/crates/algebra/src/module/quotient_module.rs @@ -1,5 +1,6 @@ use std::sync::Arc; +use anyhow::Context; use bivec::BiVec; use fp::{ matrix::Subspace, @@ -28,26 +29,48 @@ impl std::fmt::Display for QuotientModule { } impl QuotientModule { - pub fn new(module: Arc, truncation: i32) -> Self { + /// Fallible version of [`new`](Self::new). + /// + /// Returns `Err` when the allocation span `truncation + 1 - min_degree` is + /// negative or would overflow `i32` (which would trip + /// `BiVec::with_capacity`'s `debug_assert` or request a huge allocation). + /// [`new`](Self::new) is simply `Self::try_new(module, truncation).unwrap()`. + pub fn try_new(module: Arc, truncation: i32) -> anyhow::Result { + let min_degree = module.min_degree(); + let capacity = truncation + .checked_add(1) + .with_context(|| format!("truncation {truncation} + 1 overflows i32"))?; + let span = capacity + .checked_sub(min_degree) + .with_context(|| format!("span {capacity} - min_degree {min_degree} overflows i32"))?; + anyhow::ensure!( + span >= 0, + "truncation {truncation} is below min_degree - 1 ({})", + min_degree.saturating_sub(1) + ); + module.compute_basis(truncation); let p = module.prime(); - let min_degree = module.min_degree(); - let mut subspaces = BiVec::with_capacity(min_degree, truncation + 1); - let mut basis_list = BiVec::with_capacity(min_degree, truncation + 1); + let mut subspaces = BiVec::with_capacity(min_degree, capacity); + let mut basis_list = BiVec::with_capacity(min_degree, capacity); for t in min_degree..=truncation { let dim = module.dimension(t); subspaces.push(Subspace::new(p, dim)); basis_list.push((0..dim).collect()); } - Self { + Ok(Self { module, subspaces, basis_list, truncation, - } + }) + } + + pub fn new(module: Arc, truncation: i32) -> Self { + Self::try_new(module, truncation).unwrap() } pub fn quotient(&mut self, degree: i32, element: FpSlice) { @@ -203,3 +226,42 @@ impl ZeroModule for QuotientModule { Self::new(Arc::new(M::zero_module(algebra, min_degree)), min_degree) } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::{MilnorAlgebra, module::FDModule}; + + fn joker() -> Arc> { + let algebra = Arc::new(MilnorAlgebra::new(fp::prime::TWO, false)); + Arc::new(FDModule::from_json(algebra, &crate::tests::joker_json()).unwrap()) + } + + #[test] + fn try_new_valid_truncation() { + let module = joker(); + let min = module.min_degree(); + // `min_degree - 1` is the lowest valid truncation (an empty quotient). + assert!(QuotientModule::try_new(Arc::clone(&module), min - 1).is_ok()); + assert!(QuotientModule::try_new(module, min + 5).is_ok()); + } + + #[test] + fn try_new_truncation_too_low_errors() { + let module = joker(); + let min = module.min_degree(); + // Below `min_degree - 1` would request a negative-length `BiVec` capacity. + let result = QuotientModule::try_new(module, min - 2); + assert!(result.is_err()); + assert!(result.err().unwrap().to_string().contains("min_degree")); + } + + #[test] + fn try_new_truncation_overflow_errors() { + let module = joker(); + // `truncation + 1` would overflow `i32`. + let result = QuotientModule::try_new(module, i32::MAX); + assert!(result.is_err()); + assert!(result.err().unwrap().to_string().contains("overflow")); + } +} diff --git a/ext/crates/fp/src/prime/iter.rs b/ext/crates/fp/src/prime/iter.rs index ffa4f8c0a0..0dd545d37b 100644 --- a/ext/crates/fp/src/prime/iter.rs +++ b/ext/crates/fp/src/prime/iter.rs @@ -84,7 +84,7 @@ impl Iterator for BinomialIterator { fn next(&mut self) -> Option { let v = self.value; // Only available in nightly for now - #[allow(clippy::manual_isolate_lowest_one)] + #[allow(unknown_lints, clippy::manual_isolate_lowest_one)] let c = v & v.wrapping_neg(); let r = v + c; let n = (r ^ v).wrapping_shr(2 + v.trailing_zeros());