Merge pull request #51 from oflatt/oflatt-nits

oflatt · web-flow · commit 935b7854f753 · 2026-01-22T15:58:29.000-08:00
Fix nits and add toolchain file
diff --git a/rust-toolchain.toml b/rust-toolchain.toml
@@ -0,0 +1,2 @@
+[toolchain]
+channel = "1.87.0"
diff --git a/src/extract/faster_bottom_up.rs b/src/extract/faster_bottom_up.rs
@@ -69,7 +69,6 @@ Notably, insert/pop operations have O(1) expected amortized runtime complexity.
 Thanks @Bastacyclop for the implementation!
 */
 #[derive(Clone)]
-#[cfg_attr(feature = "serde-1", derive(Serialize, Deserialize))]
 pub(crate) struct UniqueQueue<T>
 where
     T: Eq + std::hash::Hash + Clone,
diff --git a/src/extract/faster_greedy_dag.rs b/src/extract/faster_greedy_dag.rs
@@ -36,7 +36,7 @@ impl FasterGreedyDagExtractor {
         let mut childrens_classes = node
             .children
             .iter()
-            .map(|c| egraph.nid_to_cid(&c).clone())
+            .map(|c| egraph.nid_to_cid(c).clone())
             .collect::<Vec<ClassId>>();
         childrens_classes.sort();
         childrens_classes.dedup();
@@ -59,19 +59,19 @@ impl FasterGreedyDagExtractor {
             .iter()
             .max_by_key(|s| costs.get(s).unwrap().costs.len())
             .unwrap();
-        let mut result = costs.get(&id_of_biggest).unwrap().costs.clone();
+        let mut result = costs.get(id_of_biggest).unwrap().costs.clone();
         for child_cid in &childrens_classes {
             if child_cid == id_of_biggest {
                 continue;
             }
 
             let next_cost = &costs.get(child_cid).unwrap().costs;
             for (key, value) in next_cost.iter() {
-                result.insert(key.clone(), value.clone());
+                result.insert(key.clone(), *value);
             }
         }
 
-        let contains = result.contains_key(&cid);
+        let contains = result.contains_key(cid);
         result.insert(cid.clone(), node.cost);
 
         let result_cost = if contains {
@@ -80,11 +80,11 @@ impl FasterGreedyDagExtractor {
             result.values().sum()
         };
 
-        return CostSet {
+        CostSet {
             costs: result,
             total: result_cost,
             choice: node_id.clone(),
-        };
+        }
     }
 }
 
@@ -151,7 +151,6 @@ Notably, insert/pop operations have O(1) expected amortized runtime complexity.
 Thanks @Bastacyclop for the implementation!
 */
 #[derive(Clone)]
-#[cfg_attr(feature = "serde-1", derive(Serialize, Deserialize))]
 pub(crate) struct UniqueQueue<T>
 where
     T: Eq + std::hash::Hash + Clone,
diff --git a/src/extract/mod.rs b/src/extract/mod.rs
@@ -82,7 +82,7 @@ impl ExtractionResult {
         }
 
         // No cycles
-        assert!(self.find_cycles(&egraph, &egraph.root_eclasses).is_empty());
+        assert!(self.find_cycles(egraph, &egraph.root_eclasses).is_empty());
 
         // Nodes should match the class they are selected into.
         for (cid, nid) in &self.choices {
diff --git a/src/extract/prio_queue.rs b/src/extract/prio_queue.rs
@@ -54,7 +54,7 @@ impl Extractor for PrioQueueExtractor {
             result.choose(class_id.clone(), node_id.clone());
             costs.insert(class_id.clone(), cost);
             for p in parents[class_id].iter() {
-                if costs.contains_key(&n2c(p)) {
+                if costs.contains_key(n2c(p)) {
                     continue;
                 }
 
@@ -79,15 +79,16 @@ mod prio {
     #[derive(PartialEq, Eq, Debug)]
     struct WithOrdRev<T: Eq, U: Ord>(pub T, pub U);
 
-    impl<T: Eq, U: Ord> PartialOrd for WithOrdRev<T, U> {
-        fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
+    impl<T: Eq, U: Ord> Ord for WithOrdRev<T, U> {
+        fn cmp(&self, other: &Self) -> Ordering {
             // It's the other way around, because we want a min-heap!
-            other.1.partial_cmp(&self.1)
+            other.1.cmp(&self.1)
         }
     }
-    impl<T: Eq, U: Ord> Ord for WithOrdRev<T, U> {
-        fn cmp(&self, other: &Self) -> Ordering {
-            self.partial_cmp(&other).unwrap()
+
+    impl<T: Eq, U: Ord> PartialOrd for WithOrdRev<T, U> {
+        fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
+            Some(self.cmp(other))
         }
     }
 
diff --git a/src/main.rs b/src/main.rs
@@ -13,17 +13,19 @@ use std::io::Write;
 use std::path::PathBuf;
 
 pub type Cost = NotNan<f64>;
-pub const INFINITY: Cost = unsafe { NotNan::new_unchecked(std::f64::INFINITY) };
+pub const INFINITY: Cost = unsafe { NotNan::new_unchecked(f64::INFINITY) };
 
 #[derive(PartialEq, Eq)]
 enum Optimal {
     Tree,
-    DAG,
+    #[cfg(feature = "ilp-cbc")]
+    Dag,
     Neither,
 }
 
 struct ExtractorDetail {
     extractor: Box<dyn Extractor>,
+    #[cfg_attr(not(test), allow(dead_code))]
     optimal: Optimal,
     use_for_bench: bool,
 }
@@ -75,7 +77,7 @@ fn extractors() -> IndexMap<&'static str, ExtractorDetail> {
             "ilp-cbc-timeout",
             ExtractorDetail {
                 extractor: extract::ilp_cbc::CbcExtractorWithTimeout::<10>.boxed(),
-                optimal: Optimal::DAG,
+                optimal: Optimal::Dag,
                 use_for_bench: true,
             },
         ),
@@ -84,7 +86,7 @@ fn extractors() -> IndexMap<&'static str, ExtractorDetail> {
             "ilp-cbc",
             ExtractorDetail {
                 extractor: extract::ilp_cbc::CbcExtractor.boxed(),
-                optimal: Optimal::DAG,
+                optimal: Optimal::Dag,
                 use_for_bench: false, // takes >10 hours sometimes
             },
         ),
@@ -93,7 +95,7 @@ fn extractors() -> IndexMap<&'static str, ExtractorDetail> {
             "faster-ilp-cbc-timeout",
             ExtractorDetail {
                 extractor: extract::faster_ilp_cbc::FasterCbcExtractorWithTimeout::<10>.boxed(),
-                optimal: Optimal::DAG,
+                optimal: Optimal::Dag,
                 use_for_bench: true,
             },
         ),
@@ -102,14 +104,14 @@ fn extractors() -> IndexMap<&'static str, ExtractorDetail> {
             "faster-ilp-cbc",
             ExtractorDetail {
                 extractor: extract::faster_ilp_cbc::FasterCbcExtractor.boxed(),
-                optimal: Optimal::DAG,
+                optimal: Optimal::Dag,
                 use_for_bench: true,
             },
         ),
     ]
     .into_iter()
     .collect();
-    return extractors;
+    extractors
 }
 
 fn main() {
diff --git a/src/test.rs b/src/test.rs
@@ -10,11 +10,11 @@ use rand::Rng;
 pub const ELABORATE_TESTING: bool = false;
 
 pub fn test_save_path(name: &str) -> String {
-    return if ELABORATE_TESTING {
+    if ELABORATE_TESTING {
         format!("/dev/shm/{}_egraph.json", name)
     } else {
         "".to_string()
-    };
+    }
 }
 
 // generates a float between 0 and 1
@@ -38,12 +38,12 @@ pub fn generate_random_egraph() -> EGraph {
     let get_semi_random_cost = |nodes: &Vec<Node>| -> Cost {
         let mut rng = rand::thread_rng();
 
-        if nodes.len() > 0 && rng.gen_bool(0.1) {
-            return nodes[rng.gen_range(0..nodes.len())].cost;
+        if !nodes.is_empty() && rng.gen_bool(0.1) {
+            nodes[rng.gen_range(0..nodes.len())].cost
         } else if rng.gen_bool(0.05) {
-            return Cost::default();
+            Cost::default()
         } else {
-            return generate_random_not_nan() * 100.0;
+            generate_random_not_nan() * 100.0
         }
     };
 
@@ -56,7 +56,7 @@ pub fn generate_random_egraph() -> EGraph {
 
         nodes.push(Node {
             op: "operation".to_string(),
-            children: children,
+            children,
             eclass: eclass.to_string().clone().into(),
             cost: get_semi_random_cost(&nodes),
         });
@@ -83,8 +83,8 @@ pub fn generate_random_egraph() -> EGraph {
 
     let mut egraph = EGraph::default();
 
-    for i in 0..nodes.len() {
-        egraph.add_node(id2nid(i), nodes[i].clone());
+    for (i, node) in nodes.iter().enumerate() {
+        egraph.add_node(id2nid(i), node.clone());
     }
 
     // Set roots
@@ -106,42 +106,65 @@ pub fn generate_random_egraph() -> EGraph {
  * Checks that the extractions are valid.
  */
 
+#[cfg(feature = "ilp-cbc")]
+fn check_dag_optimal(
+    egraph: &EGraph,
+    optimal_dag: &[Box<dyn Extractor>],
+    others: &[Box<dyn Extractor>],
+    optimal_tree_cost: Option<Cost>,
+) {
+    let mut optimal_dag_cost: Option<Cost> = None;
+
+    for e in optimal_dag {
+        let extract = e.extract(egraph, &egraph.root_eclasses);
+        extract.check(egraph);
+        let dag_cost = extract.dag_cost(egraph, &egraph.root_eclasses);
+        let tree_cost = extract.tree_cost(egraph, &egraph.root_eclasses);
+        if optimal_dag_cost.is_none() {
+            optimal_dag_cost = Some(dag_cost);
+            continue;
+        }
+
+        assert!(
+            (dag_cost.into_inner() - optimal_dag_cost.unwrap().into_inner()).abs()
+                < EPSILON_ALLOWANCE
+        );
+
+        assert!(
+            tree_cost.into_inner() + EPSILON_ALLOWANCE > optimal_dag_cost.unwrap().into_inner()
+        );
+    }
+
+    if let (Some(dag_cost), Some(tree_cost)) = (optimal_dag_cost, optimal_tree_cost) {
+        assert!(dag_cost < tree_cost + EPSILON_ALLOWANCE);
+    }
+
+    if let Some(optimal_dag_cost) = optimal_dag_cost {
+        for e in others {
+            let extract = e.extract(egraph, &egraph.root_eclasses);
+            let dag_cost = extract.dag_cost(egraph, &egraph.root_eclasses);
+            // The optimal dag should be <= any extractor's dag cost
+            assert!(optimal_dag_cost <= dag_cost + EPSILON_ALLOWANCE);
+        }
+    }
+}
+
 fn check_optimal_results<I: Iterator<Item = EGraph>>(egraphs: I) {
+    #[cfg(feature = "ilp-cbc")]
     let mut optimal_dag: Vec<Box<dyn Extractor>> = Default::default();
     let mut optimal_tree: Vec<Box<dyn Extractor>> = Default::default();
     let mut others: Vec<Box<dyn Extractor>> = Default::default();
 
     for (_, ed) in extractors().into_iter() {
         match ed.optimal {
-            Optimal::DAG => optimal_dag.push(ed.extractor),
+            #[cfg(feature = "ilp-cbc")]
+            Optimal::Dag => optimal_dag.push(ed.extractor),
             Optimal::Tree => optimal_tree.push(ed.extractor),
             Optimal::Neither => others.push(ed.extractor),
         }
     }
 
     for egraph in egraphs {
-        let mut optimal_dag_cost: Option<Cost> = None;
-
-        for e in &optimal_dag {
-            let extract = e.extract(&egraph, &egraph.root_eclasses);
-            extract.check(&egraph);
-            let dag_cost = extract.dag_cost(&egraph, &egraph.root_eclasses);
-            let tree_cost = extract.tree_cost(&egraph, &egraph.root_eclasses);
-            if optimal_dag_cost.is_none() {
-                optimal_dag_cost = Some(dag_cost);
-                continue;
-            }
-
-            assert!(
-                (dag_cost.into_inner() - optimal_dag_cost.unwrap().into_inner()).abs()
-                    < EPSILON_ALLOWANCE
-            );
-
-            assert!(
-                tree_cost.into_inner() + EPSILON_ALLOWANCE > optimal_dag_cost.unwrap().into_inner()
-            );
-        }
-
         let mut optimal_tree_cost: Option<Cost> = None;
 
         for e in &optimal_tree {
@@ -159,26 +182,19 @@ fn check_optimal_results<I: Iterator<Item = EGraph>>(egraphs: I) {
             );
         }
 
-        if optimal_dag_cost.is_some() && optimal_tree_cost.is_some() {
-            assert!(optimal_dag_cost.unwrap() < optimal_tree_cost.unwrap() + EPSILON_ALLOWANCE);
-        }
-
         for e in &others {
             let extract = e.extract(&egraph, &egraph.root_eclasses);
             extract.check(&egraph);
             let tree_cost = extract.tree_cost(&egraph, &egraph.root_eclasses);
-            let dag_cost = extract.dag_cost(&egraph, &egraph.root_eclasses);
 
             // The optimal tree cost should be <= any extractor's tree cost.
-            if optimal_tree_cost.is_some() {
-                assert!(optimal_tree_cost.unwrap() <= tree_cost + EPSILON_ALLOWANCE);
-            }
-
-            if optimal_dag_cost.is_some() {
-                // The optimal dag should be less <= any extractor's dag cost
-                assert!(optimal_dag_cost.unwrap() <= dag_cost + EPSILON_ALLOWANCE);
+            if let Some(optimal_tree_cost) = optimal_tree_cost {
+                assert!(optimal_tree_cost <= tree_cost + EPSILON_ALLOWANCE);
             }
         }
+
+        #[cfg(feature = "ilp-cbc")]
+        check_dag_optimal(&egraph, &optimal_dag, &others, optimal_tree_cost);
     }
 }
 
@@ -201,6 +217,7 @@ fn run_on_test_egraphs() {
 
 #[test]
 #[should_panic]
+#[allow(clippy::assertions_on_constants)]
 fn check_assert_enabled() {
     assert!(false);
 }
@@ -210,8 +227,8 @@ macro_rules! create_optimal_check_tests {
         $(
             #[test]
             fn $name() {
-                let optimal_dag_found = extractors().into_iter().any(|(_, ed)| ed.optimal == Optimal::DAG);
-                let iterations = if optimal_dag_found { 100 } else { 10000 };
+                // Fewer iterations when ilp-cbc is enabled since it's slow
+                let iterations = if cfg!(feature = "ilp-cbc") { 100 } else { 10000 };
                 let egraphs = (0..iterations).map(|_| generate_random_egraph());
                 check_optimal_results(egraphs);
             }

Original file line number	Diff line number	Diff line change
`@@ -82,7 +82,7 @@ impl ExtractionResult {`
`82`	`82`	`}`
`83`	`83`
`84`	`84`	`// No cycles`
`85`		`- assert!(self.find_cycles(&egraph, &egraph.root_eclasses).is_empty());`
	`85`	`+ assert!(self.find_cycles(egraph, &egraph.root_eclasses).is_empty());`
`86`	`86`
`87`	`87`	`// Nodes should match the class they are selected into.`
`88`	`88`	`for (cid, nid) in &self.choices {`
Original file line number	Diff line number	Diff line change
`@@ -54,7 +54,7 @@ impl Extractor for PrioQueueExtractor {`
`54`	`54`	`result.choose(class_id.clone(), node_id.clone());`
`55`	`55`	`costs.insert(class_id.clone(), cost);`
`56`	`56`	`for p in parents[class_id].iter() {`
`57`		`- if costs.contains_key(&n2c(p)) {`
	`57`	`+ if costs.contains_key(n2c(p)) {`
`58`	`58`	`continue;`
`59`	`59`	`}`
`60`	`60`
`@@ -79,15 +79,16 @@ mod prio {`
`79`	`79`	`#[derive(PartialEq, Eq, Debug)]`
`80`	`80`	`struct WithOrdRev<T: Eq, U: Ord>(pub T, pub U);`
`81`	`81`
`82`		`- impl<T: Eq, U: Ord> PartialOrd for WithOrdRev<T, U> {`
`83`		`- fn partial_cmp(&self, other: &Self) -> Option<Ordering> {`
	`82`	`+ impl<T: Eq, U: Ord> Ord for WithOrdRev<T, U> {`
	`83`	`+ fn cmp(&self, other: &Self) -> Ordering {`
`84`	`84`	`// It's the other way around, because we want a min-heap!`
`85`		`- other.1.partial_cmp(&self.1)`
	`85`	`+ other.1.cmp(&self.1)`
`86`	`86`	`}`
`87`	`87`	`}`
`88`		`- impl<T: Eq, U: Ord> Ord for WithOrdRev<T, U> {`
`89`		`- fn cmp(&self, other: &Self) -> Ordering {`
`90`		`- self.partial_cmp(&other).unwrap()`
	`88`	`+`
	`89`	`+ impl<T: Eq, U: Ord> PartialOrd for WithOrdRev<T, U> {`
	`90`	`+ fn partial_cmp(&self, other: &Self) -> Option<Ordering> {`
	`91`	`+ Some(self.cmp(other))`
`91`	`92`	`}`
`92`	`93`	`}`
`93`	`94`