Add try_new variants for panicking constructors

ext/crates/algebra/src/algebra/adem_algebra.rs

@@ -326,7 +326,7 @@ impl GeneratedAlgebra for AdemAlgebra {
 
     fn generators(&self, degree: i32) -> Vec<usize> {
         let p = self.prime();
-        if degree == 0 {
+        if degree <= 0 {
             return vec![];
         }
         if self.generic {

ext/crates/algebra/src/algebra/milnor_algebra.rs

@@ -695,7 +695,7 @@ impl GeneratedAlgebra for MilnorAlgebra {
     }
 
     fn generators(&self, degree: i32) -> Vec<usize> {
-        if degree == 0 {
+        if degree <= 0 {
             return vec![];
         } else if degree == 1 {
             return vec![0]; // Q_0

ext/crates/algebra/src/module/finite_dimensional_module.rs

@@ -546,7 +546,9 @@ impl<A: GeneratedAlgebra> FiniteDimensionalModule<A> {
         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<A: GeneratedAlgebra> FiniteDimensionalModule<A> {
     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());
+    }
 }

ext/crates/algebra/src/module/hom_module.rs

@@ -28,19 +28,28 @@ impl<M: Module> std::fmt::Display for HomModule<M> {
 }
 
 impl<M: Module> HomModule<M> {
-    pub fn new(source: Arc<FreeModule<M::Algebra>>, target: Arc<M>) -> 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<FreeModule<M::Algebra>>, target: Arc<M>) -> anyhow::Result<Self> {
         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<FreeModule<M::Algebra>>, target: Arc<M>) -> Self {
+        Self::try_new(source, target).unwrap()
     }
 
     pub fn source(&self) -> Arc<FreeModule<M::Algebra>> {
@@ -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"));
+    }
 }

ext/crates/algebra/src/module/homomorphism/hom_pullback.rs

@@ -23,24 +23,46 @@ pub struct HomPullback<M: Module> {
 }
 
 impl<M: Module> HomPullback<M> {
-    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<HomModule<M>>,
         target: Arc<HomModule<M>>,
         map: Arc<FreeModuleHomomorphism<FreeModule<M::Algebra>>>,
-    ) -> 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<Self> {
+        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<HomModule<M>>,
+        target: Arc<HomModule<M>>,
+        map: Arc<FreeModuleHomomorphism<FreeModule<M::Algebra>>>,
+    ) -> 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());
+    }
 }