Expose cavitation and hemolysis models to python#264
Expose cavitation and hemolysis models to python#264ryancinsight wants to merge 4 commits intomainfrom
Conversation
Exposed RayleighPlesset, CavitationRegimeClassifier, and HemolysisModel to Python via PyO3 to allow python cross-validation scripts to properly run against Rust implementations. Fixed cfd-python module imports to domain/physics. Co-authored-by: ryancinsight <55164720+ryancinsight@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
📝 WalkthroughWalkthroughAdds PyO3 Python bindings for cavitation and hemolysis, updates several import paths to a domain/physics module layout, introduces cross-validation tests comparing Rust and Python implementations, and ignores Changes
Sequence Diagram(s)sequenceDiagram
participant PyScript as Python Test Script
participant PyExt as cfd_python (PyO3 extension)
participant RustModel as Rust Physics Libraries
PyScript->>PyExt: construct PyRayleighPlesset / PyHemolysisModel
PyExt->>RustModel: create/clone underlying Rust model with params
PyScript->>PyExt: call methods (blake_critical_radius, blake_threshold, damage_index, apparent_viscosity)
PyExt->>RustModel: delegate call to Rust implementation
RustModel-->>PyExt: return numeric results
PyExt-->>PyScript: return results to Python
PyScript->>PyScript: compare with Python reference impls and assert tolerances
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request successfully exposes Rust's cavitation and hemolysis models to Python using PyO3, adding new modules and Python wrappers. The cross-validation script has been updated to use these new bindings, replacing TODOs with concrete assertions that verify the correctness of the Rust implementations. Additionally, the PR includes necessary fixes for broken import paths resulting from refactoring. The changes are well-implemented and significantly improve the project's validation capabilities.
| acoustic_pressure: Option<f64>, | ||
| acoustic_frequency: Option<f64>, | ||
| ) -> Self { | ||
| let model = bubble_model.borrow().inner.clone(); |
There was a problem hiding this comment.
The .clone() call here is redundant. The RayleighPlesset struct implements the Copy trait, so accessing the inner field on the borrowed PyRayleighPlesset already produces a copy of the value. You can remove the explicit clone for more idiomatic Rust code.
| let model = bubble_model.borrow().inner.clone(); | |
| let model = bubble_model.borrow().inner; |
![recurseml[bot]](https://avatars.githubusercontent.com/in/1054195?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review by RecurseML
🔍 Review performed on 1d431b3..62b7eee
✨ No bugs found, your code is sparkling clean
✅ Files analyzed, no issues (9)
• .gitignore
• crates/cfd-python/src/bifurcation.rs
• crates/cfd-python/src/cavitation.rs
• crates/cfd-python/src/hemolysis.rs
• crates/cfd-python/src/lib.rs
• crates/cfd-python/src/solver_2d/serpentine.rs
• crates/cfd-python/src/solver_2d/venturi.rs
• crates/cfd-python/src/womersley.rs
• validation/cross_validate_rust_python.py
Install `libfontconfig1-dev` in `.github/workflows/performance-benchmarking.yml` for Linux builds to fix the `yeslogic-fontconfig-sys` cargo build error. Co-authored-by: ryancinsight <55164720+ryancinsight@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@validation/cross_validate_rust_python.py`:
- Line 141: Remove the unnecessary f-string prefixes on print statements that
contain no formatted placeholders: replace f"\n Formula matches Python
implementation ✓" and the other static print at line ~196 with plain string
literals (e.g., "\n Formula matches Python implementation ✓") so the prints are
regular strings and avoid Ruff F541; search for print(...) calls in
cross_validate_rust_python.py that use an f-prefix but contain no braces and
remove the leading 'f'.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a70af1ba-51e0-48ea-a657-9aa0064ea3a8
📒 Files selected for processing (9)
.gitignorecrates/cfd-python/src/bifurcation.rscrates/cfd-python/src/cavitation.rscrates/cfd-python/src/hemolysis.rscrates/cfd-python/src/lib.rscrates/cfd-python/src/solver_2d/serpentine.rscrates/cfd-python/src/solver_2d/venturi.rscrates/cfd-python/src/womersley.rsvalidation/cross_validate_rust_python.py
| # Assert they match | ||
| assert error_pct < 0.01, f"Viscosity mismatch at {gamma_dot} s⁻¹" | ||
|
|
||
| print(f"\n Formula matches Python implementation ✓") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
import ast
from pathlib import Path
path = Path("validation/cross_validate_rust_python.py")
source = path.read_text()
tree = ast.parse(source)
for node in ast.walk(tree):
if isinstance(node, ast.JoinedStr) and node.lineno in {141, 196}:
has_formatted = any(isinstance(value, ast.FormattedValue) for value in node.values)
print(f"Line {node.lineno}: has_formatted={has_formatted} :: {ast.get_source_segment(source, node)}")
PYRepository: ryancinsight/CFDrs
Length of output: 221
Remove unnecessary f prefix from string literals without formatted values.
Lines 141 and 196 contain f-strings without any formatted placeholders, which triggers Ruff's F541 violation. These should be plain string literals.
Fix
- print(f"\n Formula matches Python implementation ✓")
+ print("\n Formula matches Python implementation ✓")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| print(f"\n Formula matches Python implementation ✓") | |
| print("\n Formula matches Python implementation ✓") |
🧰 Tools
🪛 Ruff (0.15.5)
[error] 141-141: f-string without any placeholders
Remove extraneous f prefix
(F541)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@validation/cross_validate_rust_python.py` at line 141, Remove the unnecessary
f-string prefixes on print statements that contain no formatted placeholders:
replace f"\n Formula matches Python implementation ✓" and the other static
print at line ~196 with plain string literals (e.g., "\n Formula matches Python
implementation ✓") so the prints are regular strings and avoid Ruff F541; search
for print(...) calls in cross_validate_rust_python.py that use an f-prefix but
contain no braces and remove the leading 'f'.
- Define a fallback `BenchmarkConfig` structure inside benchmarks if `cfd-validation` is disabled via features (the `validation` workspace feature). - Fix benchmark map closure typing issues. Co-authored-by: ryancinsight <55164720+ryancinsight@users.noreply.github.com>
Co-authored-by: ryancinsight <55164720+ryancinsight@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@benches/comprehensive_cfd_benchmarks.rs`:
- Around line 8-31: The fallback BenchmarkConfig struct and its Default impl are
missing the baseline_data and output_dir fields, breaking compatibility with
cfd_validation::benchmarking::BenchmarkConfig; update the struct
(BenchmarkConfig) to include pub baseline_data: Option<...> (matching the
canonical type, e.g., Option<BenchmarkBaseline> or Option<String> as
appropriate) and pub output_dir: PathBuf or String per the canonical API, then
set sensible defaults in impl Default for BenchmarkConfig (e.g., baseline_data:
None and output_dir: default directory like PathBuf::from("bench_output") or
String::new()) so code paths that reference output_dir and baseline_data (e.g.,
memory_profiling.rs usage) compile when validation feature is disabled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: edfd2f82-ef29-4c57-b314-db4f2c308a63
📒 Files selected for processing (4)
benches/cfd_suite/core_operations.rsbenches/cfd_suite/memory_profiling.rsbenches/cfd_suite/scaling_analysis.rsbenches/comprehensive_cfd_benchmarks.rs
| #[cfg(not(feature = "validation"))] | ||
| #[derive(Debug, Clone)] | ||
| pub struct BenchmarkConfig { | ||
| pub iterations: usize, | ||
| pub warmup_iterations: usize, | ||
| pub enable_memory: bool, | ||
| pub enable_scaling: bool, | ||
| pub detailed_reporting: bool, | ||
| pub regression_threshold: f64, | ||
| pub problem_sizes: Vec<usize>, | ||
| } | ||
| #[cfg(not(feature = "validation"))] | ||
| impl Default for BenchmarkConfig { | ||
| fn default() -> Self { | ||
| Self { | ||
| iterations: 10, | ||
| warmup_iterations: 3, | ||
| enable_memory: true, | ||
| enable_scaling: true, | ||
| detailed_reporting: true, | ||
| regression_threshold: 5.0, | ||
| problem_sizes: vec![32, 64, 128, 256, 512], | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
import pathlib, re
def extract_fields(path_str):
path = pathlib.Path(path_str)
text = path.read_text()
m = re.search(r'pub struct BenchmarkConfig\s*\{(.*?)\n\}', text, re.S)
if not m:
raise SystemExit(f"BenchmarkConfig not found in {path}")
fields = []
for line in m.group(1).splitlines():
line = line.strip()
if line.startswith("pub "):
fields.append(line.split(":")[0].replace("pub ", "").strip())
return fields
local_path = "benches/comprehensive_cfd_benchmarks.rs"
canonical_path = "crates/cfd-validation/src/benchmarking/suite.rs"
local_fields = extract_fields(local_path)
canonical_fields = extract_fields(canonical_path)
print("local fields :", local_fields)
print("canonical fields :", canonical_fields)
print("missing in local :", sorted(set(canonical_fields) - set(local_fields)))
print("extra in local :", sorted(set(local_fields) - set(canonical_fields)))
PY
rg -n -C2 '\b(baseline_data|output_dir)\b' benches crates/cfd-validation/src/benchmarkingRepository: ryancinsight/CFDrs
Length of output: 5569
Add missing baseline_data and output_dir fields to the fallback BenchmarkConfig.
The local fallback struct omits baseline_data and output_dir from the canonical cfd_validation::benchmarking::BenchmarkConfig, breaking API compatibility. The output_dir field is actively used in benches/cfd_suite/memory_profiling.rs (lines 635–639, 768–769), so code fails when the validation feature is disabled.
Fix
#[cfg(not(feature = "validation"))]
#[derive(Debug, Clone)]
pub struct BenchmarkConfig {
pub iterations: usize,
pub warmup_iterations: usize,
pub enable_memory: bool,
pub enable_scaling: bool,
pub detailed_reporting: bool,
pub regression_threshold: f64,
+ pub baseline_data: Option<std::collections::HashMap<String, f64>>,
pub problem_sizes: Vec<usize>,
+ pub output_dir: Option<String>,
}
#[cfg(not(feature = "validation"))]
impl Default for BenchmarkConfig {
fn default() -> Self {
Self {
iterations: 10,
warmup_iterations: 3,
enable_memory: true,
enable_scaling: true,
detailed_reporting: true,
regression_threshold: 5.0,
+ baseline_data: None,
problem_sizes: vec![32, 64, 128, 256, 512],
+ output_dir: Some("performance_results".to_string()),
}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[cfg(not(feature = "validation"))] | |
| #[derive(Debug, Clone)] | |
| pub struct BenchmarkConfig { | |
| pub iterations: usize, | |
| pub warmup_iterations: usize, | |
| pub enable_memory: bool, | |
| pub enable_scaling: bool, | |
| pub detailed_reporting: bool, | |
| pub regression_threshold: f64, | |
| pub problem_sizes: Vec<usize>, | |
| } | |
| #[cfg(not(feature = "validation"))] | |
| impl Default for BenchmarkConfig { | |
| fn default() -> Self { | |
| Self { | |
| iterations: 10, | |
| warmup_iterations: 3, | |
| enable_memory: true, | |
| enable_scaling: true, | |
| detailed_reporting: true, | |
| regression_threshold: 5.0, | |
| problem_sizes: vec![32, 64, 128, 256, 512], | |
| } | |
| } | |
| #[cfg(not(feature = "validation"))] | |
| #[derive(Debug, Clone)] | |
| pub struct BenchmarkConfig { | |
| pub iterations: usize, | |
| pub warmup_iterations: usize, | |
| pub enable_memory: bool, | |
| pub enable_scaling: bool, | |
| pub detailed_reporting: bool, | |
| pub regression_threshold: f64, | |
| pub baseline_data: Option<std::collections::HashMap<String, f64>>, | |
| pub problem_sizes: Vec<usize>, | |
| pub output_dir: Option<String>, | |
| } | |
| #[cfg(not(feature = "validation"))] | |
| impl Default for BenchmarkConfig { | |
| fn default() -> Self { | |
| Self { | |
| iterations: 10, | |
| warmup_iterations: 3, | |
| enable_memory: true, | |
| enable_scaling: true, | |
| detailed_reporting: true, | |
| regression_threshold: 5.0, | |
| baseline_data: None, | |
| problem_sizes: vec![32, 64, 128, 256, 512], | |
| output_dir: Some("performance_results".to_string()), | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@benches/comprehensive_cfd_benchmarks.rs` around lines 8 - 31, The fallback
BenchmarkConfig struct and its Default impl are missing the baseline_data and
output_dir fields, breaking compatibility with
cfd_validation::benchmarking::BenchmarkConfig; update the struct
(BenchmarkConfig) to include pub baseline_data: Option<...> (matching the
canonical type, e.g., Option<BenchmarkBaseline> or Option<String> as
appropriate) and pub output_dir: PathBuf or String per the canonical API, then
set sensible defaults in impl Default for BenchmarkConfig (e.g., baseline_data:
None and output_dir: default directory like PathBuf::from("bench_output") or
String::new()) so code paths that reference output_dir and baseline_data (e.g.,
memory_profiling.rs usage) compile when validation feature is disabled.
1 participant
Adds missing Python PyO3 bindings for Blake threshold calculation and Giersiepen Hemolysis Model, which had been marked with TODO in the python validation code
validation/cross_validate_rust_python.py. The python script has been updated to use these bindings, asserting output exactness, and printingFormula matches Python implementation ✓correctly. Also addresses some broken internalcfd-pythoncrate imports forcfd-1d.PR created automatically by Jules for task 1581635658287821159 started by @ryancinsight
High-level PR Summary
This PR exposes cavitation (Blake threshold calculation) and hemolysis (Giersiepen damage model) functionality from Rust to Python via PyO3 bindings. The new Python bindings for
RayleighPlesset,CavitationRegimeClassifier, andHemolysisModelare integrated into thecfd-pythoncrate and validated through the cross-validation script, which now performs exact output comparisons and confirms formula correctness. The PR also fixes broken internal imports in thecfd-pythoncrate related tocfd-1dmodule reorganization and adds.venvto.gitignore.⏱️ Estimated Review Time: 5-15 minutes
💡 Review Order Suggestion
crates/cfd-python/src/cavitation.rscrates/cfd-python/src/hemolysis.rscrates/cfd-python/src/lib.rsvalidation/cross_validate_rust_python.pycrates/cfd-python/src/bifurcation.rscrates/cfd-python/src/solver_2d/serpentine.rscrates/cfd-python/src/solver_2d/venturi.rscrates/cfd-python/src/womersley.rs.gitignoreSummary by CodeRabbit
New Features
Tests
Chores