From 1df83f34f3ff32c44ff990de706ae1c28ca59638 Mon Sep 17 00:00:00 2001
From: Adam Getchell <acgetchell@ucdavis.edu>
Date: Sat, 9 Nov 2024 12:38:50 -0800
Subject: [PATCH] fix: CodeRabbit suggestions

---
 src/delaunay_core/cell.rs                         | 14 ++++++++++----
 src/delaunay_core/facet.rs                        | 10 ++++++----
 src/delaunay_core/triangulation_data_structure.rs | 10 +++++-----
 3 files changed, 21 insertions(+), 13 deletions(-)

diff --git a/src/delaunay_core/cell.rs b/src/delaunay_core/cell.rs
index ddb8c07..cb7e389 100644
--- a/src/delaunay_core/cell.rs
+++ b/src/delaunay_core/cell.rs
@@ -247,8 +247,8 @@ where
     /// let cell: Cell<f64, i32, &str, 3> = CellBuilder::default().vertices(vec![vertex1, vertex2, vertex3, vertex4]).data("three-one cell").build().unwrap();
     /// let vertex5: Vertex<f64, i32, 3> = VertexBuilder::default().point(Point::new([0.0, 0.0, 0.0])).data(0).build().unwrap();
     /// let cell2: Cell<f64, i32, &str, 3> = CellBuilder::default().vertices(vec![vertex1, vertex2, vertex3, vertex5]).data("one-three cell").build().unwrap();
-    /// assert!(cell.contains_vertex_of(cell2));
-    pub fn contains_vertex_of(&self, cell: Cell<T, U, V, D>) -> bool {
+    /// assert!(cell.contains_vertex_of(&cell2));
+    pub fn contains_vertex_of(&self, cell: &Cell<T, U, V, D>) -> bool {
         self.vertices.iter().any(|v| cell.vertices.contains(v))
     }
 
@@ -406,7 +406,9 @@ where
 
     /// The function `circumsphere_contains_vertex` checks if a given vertex is
     /// contained in the circumsphere of the Cell using a matrix determinant.
-    /// It should be more numerically stable than `circumsphere_contains`.
+    /// This method is preferred over `circumsphere_contains` as it provides better numerical
+    /// stability by using a matrix determinant approach instead of distance calculations,
+    /// which can accumulate floating-point errors.
     ///
     /// # Arguments:
     ///
@@ -499,6 +501,10 @@ where
     /// Returns `true` if the given facet is contained in the cell, and `false` otherwise.
     pub fn contains_facet(&self, facet: &Facet<T, U, V, D>) -> bool {
         self.vertices.iter().all(|v| facet.vertices().contains(v))
+
+        // Alternative implementation using HashSet, requires Eq on T in Vertex<T, U, D>
+        // let vertex_set: HashSet<&Vertex<T, U, D>> = self.vertices.iter().collect();
+        // facet.vertices().iter().all(|v| vertex_set.contains(v))
     }
 }
 
@@ -901,7 +907,7 @@ mod tests {
             .build()
             .unwrap();
 
-        assert!(cell.contains_vertex_of(cell2));
+        assert!(cell.contains_vertex_of(&cell2));
 
         // Human readable output for cargo test -- --nocapture
         println!("Cell: {:?}", cell);
diff --git a/src/delaunay_core/facet.rs b/src/delaunay_core/facet.rs
index e584ed8..ce2b0c7 100644
--- a/src/delaunay_core/facet.rs
+++ b/src/delaunay_core/facet.rs
@@ -88,10 +88,12 @@ where
     /// The `vertices` method in the [Facet] returns a container of
     /// [Vertex] objects that are in the [Facet].
     pub fn vertices(&self) -> Vec<Vertex<T, U, D>> {
-        let mut vertices = self.cell.clone().vertices;
-        vertices.retain(|v| *v != self.vertex);
-
-        vertices
+        self.cell
+            .vertices
+            .iter()
+            .filter(|v| **v != self.vertex)
+            .cloned()
+            .collect()
     }
 }
 
diff --git a/src/delaunay_core/triangulation_data_structure.rs b/src/delaunay_core/triangulation_data_structure.rs
index 292fed4..9d611e3 100644
--- a/src/delaunay_core/triangulation_data_structure.rs
+++ b/src/delaunay_core/triangulation_data_structure.rs
@@ -263,14 +263,14 @@ where
         self.cells.insert(supercell.uuid, supercell.clone());
 
         // Iterate over vertices
-        for vertex in self.vertices.values().cloned().collect::<Vec<_>>() {
+        for vertex in self.vertices.values() {
             let mut bad_cells = Vec::new();
             let mut boundary_facets = Vec::new();
 
             // Find cells whose circumsphere contains the vertex
             for (cell_id, cell) in self.cells.iter() {
                 // if cell.circumsphere_contains(vertex)? {
-                if cell.circumsphere_contains_vertex(vertex)? {
+                if cell.circumsphere_contains_vertex(*vertex)? {
                     bad_cells.push(*cell_id);
                 }
             }
@@ -297,14 +297,14 @@ where
 
             // Create new cells using the boundary facets and the new vertex
             for facet in boundary_facets {
-                let new_cell = Cell::from_facet_and_vertex(facet, vertex)?;
+                let new_cell = Cell::from_facet_and_vertex(facet, *vertex)?;
                 self.cells.insert(new_cell.uuid, new_cell);
             }
         }
 
         // Remove cells that contain vertices of the supercell
         self.cells
-            .retain(|_, cell| !cell.contains_vertex_of(supercell.clone()));
+            .retain(|_, cell| !cell.contains_vertex_of(&supercell));
 
         Ok(self.clone())
     }
@@ -466,7 +466,7 @@ mod tests {
         });
 
         assert_eq!(result.number_of_vertices(), 4);
-        assert_eq!(result.number_of_cells(), 0);
+        assert_eq!(result.number_of_cells(), 1);
 
         // Human readable output for cargo test -- --nocapture
         println!("{:?}", result);