imbajin commented on code in PR #359: URL: https://github.com/apache/hugegraph-computer/pull/359#discussion_r3897617821
########## computer-rust/src/kernel/pagerank.rs: ########## @@ -0,0 +1,135 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +use crate::kernel::csr::CsrGraph; + +#[derive(Debug, Clone)] +pub struct PageRankKernel { + damping_factor: f64, + max_iterations: u32, + tolerance: f64, +} + +impl PageRankKernel { + pub fn try_new( + damping_factor: f64, + max_iterations: u32, + tolerance: f64, + ) -> Result<Self, String> { + if !damping_factor.is_finite() || damping_factor < 0.0 || damping_factor > 1.0 { + return Err(format!( + "Invalid damping_factor: {}. Must be finite and in range [0.0, 1.0]", + damping_factor + )); + } + if !tolerance.is_finite() || tolerance < 0.0 { + return Err(format!( + "Invalid tolerance: {}. Must be finite non-negative number", + tolerance + )); + } + Ok(Self { + damping_factor, + max_iterations, + tolerance, + }) + } + + pub fn new(damping_factor: f64, max_iterations: u32, tolerance: f64) -> Self { + Self::try_new(damping_factor, max_iterations, tolerance) + .expect("Failed to initialize PageRankKernel due to invalid parameters") + } + + #[allow(clippy::needless_range_loop)] + pub fn compute(&self, graph: &CsrGraph) -> Vec<f64> { + let num_vertices = graph.num_vertices() as usize; + if num_vertices == 0 { + return Vec::new(); + } + + let initial_rank = 1.0 / (num_vertices as f64); + let mut ranks = vec![initial_rank; num_vertices]; + let mut next_ranks = vec![0.0; num_vertices]; + + let teleport = (1.0 - self.damping_factor) / (num_vertices as f64); + + for _iter in 0..self.max_iterations { + next_ranks.fill(0.0); + let mut dangling_sum = 0.0; + + for v in 0..num_vertices { + let out_degree = graph.out_degree(v as u32); + if out_degree == 0 { + dangling_sum += ranks[v]; + } else { + let share = ranks[v] / (out_degree as f64); + let (neighbors, _) = graph.out_edges(v as u32); Review Comment: ⚠️ The C API accepts a `weight` and CsrGraph stores it, but this binding explicitly discards the weights (`let (neighbors, _)`) and always sends `ranks[v] / out_degree` to every neighbor. Consequently, edges with weights 1.0 and 100.0 produce the same PageRank result, despite the exported API and CSR carrying edge weights. Please either use normalized outgoing weights in the transition or remove/document the argument as topology-only, and add an asymmetric-weight regression test. Evidence: computer-rust/src/ffi/c_api.rs:45-64, computer-rust/src/kernel/csr.rs:60-73, and this line. ########## computer-rust/src/fixtures/tolerance.rs: ########## @@ -0,0 +1,100 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +pub struct DifferentialTolerance; + +impl DifferentialTolerance { + pub fn l1_distance(actual: &[f64], expected: &[f64]) -> Result<f64, String> { + if actual.len() != expected.len() { + return Err(format!( + "Vector length mismatch: actual len {}, expected len {}", + actual.len(), + expected.len() + )); + } + + let mut l1 = 0.0; + for i in 0..actual.len() { + if !actual[i].is_finite() || !expected[i].is_finite() { + return Err(format!( + "Non-finite value detected at index {}: actual = {}, expected = {}", + i, actual[i], expected[i] + )); + } + l1 += (actual[i] - expected[i]).abs(); + } + + if !l1.is_finite() { + return Err("Calculated L1 distance is non-finite".to_string()); + } + + Ok(l1) + } + + pub fn assert_parity(actual: &[f64], expected: &[f64], epsilon: f64) -> Result<(), String> { + if actual.len() != expected.len() { + return Err(format!( + "Vector length mismatch: actual len {}, expected len {}", + actual.len(), + expected.len() + )); + } + + for i in 0..actual.len() { + if !actual[i].is_finite() || !expected[i].is_finite() { + return Err(format!( + "Non-finite value detected at index {}: actual = {}, expected = {}", + i, actual[i], expected[i] + )); + } + let diff = (actual[i] - expected[i]).abs(); + if !diff.is_finite() || diff > epsilon { Review Comment: ⚠️ `assert_parity` checks each element against `epsilon`, but the roadmap promises an L1-distance bound. For two elements whose absolute differences are each 0.75 * epsilon, this function returns Ok while the L1 distance is 1.5 * epsilon; a caller can therefore accept a result outside the advertised contract. Please compare `l1_distance(actual, expected)` with a validated epsilon, or rename/document this as a per-element check, and add a multi-element regression test. Evidence: the per-element `diff > epsilon` condition at this line and the L1 contract in docs/rust-modernization-roadmap.md. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
