Implement PyO3 bindings for Blake Threshold and Giersiepen Hemolysis#262
Implement PyO3 bindings for Blake Threshold and Giersiepen Hemolysis#262ryancinsight wants to merge 2 commits intomainfrom
Conversation
- Added `cavitation.rs` and `hemolysis.rs` into `crates/cfd-python/src/` - Implemented `PyBlakeThreshold` and `PyGiersiepenModel` inside these modules - Registered these new PyClasses inside `cfd_python` module initialization in `crates/cfd-python/src/lib.rs` - Fixed module resolution inside `crates/cfd-python/src/` resolving import errors for python bindings - Removed TODO flags in `validation/cross_validate_rust_python.py` - Setup logic in `validation/cross_validate_rust_python.py` using `cfd_python` to fetch variables generated by rust implementations to verify equivalency with python variables 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR updates several cfd_1d import paths to new domain/physics locations, adds PyO3 bindings for BlakeThreshold (cavitation) and GiersiepenModel (hemolysis), registers them in the Python module, and enhances the cross-validation script to compare Rust bindings against Python implementations. Changes
Sequence Diagram(s)sequenceDiagram
participant Validator as Python Validator Script
participant PyBind as cfd_python (PyO3 bindings)
participant RustModel as Rust physics/hemolysis models
Validator->>PyBind: instantiate BlakeThreshold(ambient_pressure)
PyBind->>RustModel: build CavitationRegimeClassifier / RayleighPlesset
RustModel-->>PyBind: return classifier & threshold functions
PyBind-->>Validator: provide critical_radius() and threshold()
Validator->>Validator: compute reference Python values
Validator->>PyBind: call threshold()/critical_radius()/calculate_damage()
Validator->>Validator: compare Rust vs Python, print percent diffs and PASS/MISMATCH
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
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 expands 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
|
![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 implements PyO3 bindings for the Blake Threshold and Giersiepen Hemolysis models, and adds the corresponding cross-validation tests against the Python implementations. The changes are well-structured and the validation script is now more comprehensive. I have one suggestion in cavitation.rs to improve maintainability by reducing code duplication.
| fn critical_radius(&self) -> f64 { | ||
| let model = RayleighPlesset { | ||
| initial_radius: 10e-6, | ||
| liquid_density: 997.0, | ||
| liquid_viscosity: 0.001, | ||
| surface_tension: 0.0728, | ||
| vapor_pressure: 2339.0, | ||
| polytropic_index: 1.4, | ||
| }; | ||
| model.blake_critical_radius(self.ambient_pressure) | ||
| } |
There was a problem hiding this comment.
The RayleighPlesset model is re-created here with the same hardcoded values used in the new function. This code duplication can lead to maintenance issues. For instance, if the model parameters in new are updated, the same change would need to be manually applied here.
A better approach would be to store the RayleighPlesset instance on the PyBlakeThreshold struct itself and reuse it. Since RayleighPlesset implements Copy, this is an efficient change.
You could refactor this by:
- Adding a
model: RayleighPlesset<f64>field toPyBlakeThreshold. - In
new(), create themodelonce and store it on the newPyBlakeThresholdinstance. TheCavitationRegimeClassifiercan be created using this samemodelinstance. - This
critical_radiusfunction can then be simplified to justself.model.blake_critical_radius(self.ambient_pressure).
This would centralize the model creation and make the code more robust.
- Added `libfontconfig1-dev` and `pkg-config` dependencies to `.github/workflows/performance-benchmarking.yml` for Linux runners because `yeslogic-fontconfig-sys` relies on them to compile, addressing CI build failures. Co-authored-by: ryancinsight <55164720+ryancinsight@users.noreply.github.com>
![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..ab26b64
✨ No bugs found, your code is sparkling clean
✅ Files analyzed, no issues (8)
• 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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
crates/cfd-python/src/cavitation.rs (1)
32-43: Eliminate duplicatedRayleighPlessetinitialization.The same model parameters are hardcoded in both
new()(lines 17-24) andcritical_radius()(lines 34-41). Consider extracting to a helper function or accessing the model stored in the classifier.♻️ Suggested refactor to reduce duplication
+fn default_rayleigh_plesset() -> RayleighPlesset<f64> { + RayleighPlesset { + initial_radius: 10e-6, + liquid_density: 997.0, + liquid_viscosity: 0.001, + surface_tension: 0.0728, + vapor_pressure: 2339.0, + polytropic_index: 1.4, + } +} + /// Calculator for Blake Threshold for cavitation #[pyclass(name = "BlakeThreshold")] pub struct PyBlakeThreshold { classifier: CavitationRegimeClassifier<f64>, ambient_pressure: f64, + model: RayleighPlesset<f64>, } #[pymethods] impl PyBlakeThreshold { #[new] fn new(ambient_pressure: f64) -> Self { - let model = RayleighPlesset { - initial_radius: 10e-6, - liquid_density: 997.0, - liquid_viscosity: 0.001, - surface_tension: 0.0728, - vapor_pressure: 2339.0, - polytropic_index: 1.4, - }; + let model = default_rayleigh_plesset(); PyBlakeThreshold { - classifier: CavitationRegimeClassifier::new(model, ambient_pressure, None, None), + classifier: CavitationRegimeClassifier::new(model.clone(), ambient_pressure, None, None), ambient_pressure, + model, } } fn critical_radius(&self) -> f64 { - let model = RayleighPlesset { - initial_radius: 10e-6, - liquid_density: 997.0, - liquid_viscosity: 0.001, - surface_tension: 0.0728, - vapor_pressure: 2339.0, - polytropic_index: 1.4, - }; - model.blake_critical_radius(self.ambient_pressure) + self.model.blake_critical_radius(self.ambient_pressure) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cfd-python/src/cavitation.rs` around lines 32 - 43, critical_radius currently reconstructs a RayleighPlesset with hardcoded parameters duplicated from new(); remove the duplication by extracting the shared initialization into a single helper and using it from both places—either add a field (e.g., rayleigh_model: RayleighPlesset) to the struct initialized in new() and call that field in critical_radius(), or implement a private method like fn rayleigh_plesset_model(&self) -> RayleighPlesset that returns the configured model and call it from new() and critical_radius(); update critical_radius to call the helper/field (instead of re-creating the model) and adjust new()/struct definition accordingly.
🤖 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`:
- Around line 172-192: Two print statements use f-strings without placeholders —
remove the unnecessary 'f' prefix to avoid misleading code and tiny overhead:
change the print(f"\nRust implementation (via cfd_python.GiersiepenModel):") and
print(f"\nComparison:") to plain print(...) calls; keep all other prints that
use formatting (like the formatted table and diff calculations using
giersiepen_python and hemolysis_model.calculate_damage) unchanged.
- Around line 57-75: Change the two print calls that use f-strings but have no
interpolations to plain strings: replace print(f"\nRust implementation (via
cfd_python.BlakeThreshold):") and replace print(f"\nComparison:") with
print("\nRust implementation (via cfd_python.BlakeThreshold):") and
print("\nComparison:") respectively; keep all other f-strings (e.g., the ones
printing R_c, P_Blake, rc_diff, pb_diff) unchanged.
- Around line 115-134: The two print statements using unnecessary f-strings
should be changed to plain string literals: remove the leading `f` from the
print that prints the Rust implementation header (the print currently before
creating `cy_blood = cfd_python.CarreauYasudaBlood()`) and from the print that
outputs the comparison header (the print before checking `all_match`); keep all
other prints (those that format variables like `gamma_dot`, `mu_rust`, and
`diff_pct`) as-is so `test_shear_rates`, `carreau_yasuda_python`, and
`cy_blood.viscosity` usage remains unchanged.
---
Nitpick comments:
In `@crates/cfd-python/src/cavitation.rs`:
- Around line 32-43: critical_radius currently reconstructs a RayleighPlesset
with hardcoded parameters duplicated from new(); remove the duplication by
extracting the shared initialization into a single helper and using it from both
places—either add a field (e.g., rayleigh_model: RayleighPlesset) to the struct
initialized in new() and call that field in critical_radius(), or implement a
private method like fn rayleigh_plesset_model(&self) -> RayleighPlesset that
returns the configured model and call it from new() and critical_radius();
update critical_radius to call the helper/field (instead of re-creating the
model) and adjust new()/struct definition accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6ceacc20-c978-4cf1-a4a0-d9ae99777759
📒 Files selected for processing (8)
crates/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
| print(f"\nRust implementation (via cfd_python.BlakeThreshold):") | ||
| blake_calculator = cfd_python.BlakeThreshold(ambient_pressure=P_inf) | ||
| R_c_rust = blake_calculator.critical_radius() | ||
| P_Blake_rust = blake_calculator.threshold() | ||
|
|
||
| print(f" R_c = {R_c_rust*1e6:.4f} μm") | ||
| print(f" P_Blake = {P_Blake_rust:.2f} Pa = {P_Blake_rust/1000:.2f} kPa") | ||
|
|
||
| rc_diff = abs(R_c_rust - R_c_python) / R_c_python * 100 | ||
| pb_diff = abs(P_Blake_rust - P_Blake_python) / P_Blake_python * 100 | ||
|
|
||
| print(f"\nComparison:") | ||
| print(f" R_c diff: {rc_diff:.4e}%") | ||
| print(f" P_Blake diff: {pb_diff:.4e}%") | ||
|
|
||
| if rc_diff < 1e-4 and pb_diff < 1e-4: | ||
| print(" ✓ MATCH") | ||
| else: | ||
| print(" ✗ MISMATCH") |
There was a problem hiding this comment.
Remove unnecessary f-string prefixes.
Lines 57 and 68 use f-strings without placeholders. Use regular strings instead.
🧹 Proposed fix
- print(f"\nRust implementation (via cfd_python.BlakeThreshold):")
+ print("\nRust implementation (via cfd_python.BlakeThreshold):")
blake_calculator = cfd_python.BlakeThreshold(ambient_pressure=P_inf)
R_c_rust = blake_calculator.critical_radius()
P_Blake_rust = blake_calculator.threshold()
print(f" R_c = {R_c_rust*1e6:.4f} μm")
print(f" P_Blake = {P_Blake_rust:.2f} Pa = {P_Blake_rust/1000:.2f} kPa")
rc_diff = abs(R_c_rust - R_c_python) / R_c_python * 100
pb_diff = abs(P_Blake_rust - P_Blake_python) / P_Blake_python * 100
- print(f"\nComparison:")
+ print("\nComparison:")📝 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"\nRust implementation (via cfd_python.BlakeThreshold):") | |
| blake_calculator = cfd_python.BlakeThreshold(ambient_pressure=P_inf) | |
| R_c_rust = blake_calculator.critical_radius() | |
| P_Blake_rust = blake_calculator.threshold() | |
| print(f" R_c = {R_c_rust*1e6:.4f} μm") | |
| print(f" P_Blake = {P_Blake_rust:.2f} Pa = {P_Blake_rust/1000:.2f} kPa") | |
| rc_diff = abs(R_c_rust - R_c_python) / R_c_python * 100 | |
| pb_diff = abs(P_Blake_rust - P_Blake_python) / P_Blake_python * 100 | |
| print(f"\nComparison:") | |
| print(f" R_c diff: {rc_diff:.4e}%") | |
| print(f" P_Blake diff: {pb_diff:.4e}%") | |
| if rc_diff < 1e-4 and pb_diff < 1e-4: | |
| print(" ✓ MATCH") | |
| else: | |
| print(" ✗ MISMATCH") | |
| print("\nRust implementation (via cfd_python.BlakeThreshold):") | |
| blake_calculator = cfd_python.BlakeThreshold(ambient_pressure=P_inf) | |
| R_c_rust = blake_calculator.critical_radius() | |
| P_Blake_rust = blake_calculator.threshold() | |
| print(f" R_c = {R_c_rust*1e6:.4f} μm") | |
| print(f" P_Blake = {P_Blake_rust:.2f} Pa = {P_Blake_rust/1000:.2f} kPa") | |
| rc_diff = abs(R_c_rust - R_c_python) / R_c_python * 100 | |
| pb_diff = abs(P_Blake_rust - P_Blake_python) / P_Blake_python * 100 | |
| print("\nComparison:") | |
| print(f" R_c diff: {rc_diff:.4e}%") | |
| print(f" P_Blake diff: {pb_diff:.4e}%") | |
| if rc_diff < 1e-4 and pb_diff < 1e-4: | |
| print(" ✓ MATCH") | |
| else: | |
| print(" ✗ MISMATCH") |
🧰 Tools
🪛 Ruff (0.15.4)
[error] 57-57: f-string without any placeholders
Remove extraneous f prefix
(F541)
[error] 68-68: 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` around lines 57 - 75, Change the
two print calls that use f-strings but have no interpolations to plain strings:
replace print(f"\nRust implementation (via cfd_python.BlakeThreshold):") and
replace print(f"\nComparison:") with print("\nRust implementation (via
cfd_python.BlakeThreshold):") and print("\nComparison:") respectively; keep all
other f-strings (e.g., the ones printing R_c, P_Blake, rc_diff, pb_diff)
unchanged.
| print(f"\nRust implementation (via cfd_python.CarreauYasudaBlood):") | ||
| cy_blood = cfd_python.CarreauYasudaBlood() | ||
|
|
||
| # Try to test if we can create a blood model | ||
| # Note: This depends on cfd_python API structure | ||
| print(f"\n TODO: Add cfd_python API test if blood model is exposed") | ||
| print(f"{'Shear Rate (s⁻¹)':>20} {'μ_rust (mPa·s)':>15} {'Diff %':>15}") | ||
| print("-" * 55) | ||
|
|
||
| all_match = True | ||
| for gamma_dot in test_shear_rates: | ||
| mu_python = carreau_yasuda_python(gamma_dot) | ||
| mu_rust = cy_blood.viscosity(gamma_dot) | ||
| diff_pct = abs(mu_rust - mu_python) / mu_python * 100 | ||
| print(f"{gamma_dot:20.0f} {mu_rust * 1000:15.4f} {diff_pct:14.4e}%") | ||
| if diff_pct > 1e-4: | ||
| all_match = False | ||
|
|
||
| print(f"\nComparison:") | ||
| if all_match: | ||
| print(" ✓ MATCH") | ||
| else: | ||
| print(" ✗ MISMATCH") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if the Rust CarreauYasudaBlood uses the same default parameters as the Python validation script
# Search for Carreau-Yasuda default parameters in cfd-core
rg -n "normal_blood|zero_shear|infinite_shear|lambda|relaxation" --type rust crates/cfd-core/src/physics/fluid/
# Check the blood.rs file specifically for default values
ast-grep --pattern 'fn normal_blood() -> Self {
$$$
}'Repository: ryancinsight/CFDrs
Length of output: 8534
🏁 Script executed:
# Get constant values
cat -n crates/cfd-core/src/physics/fluid/blood/constants.rs | head -50
# Check the validation file for f-string usage and parameters
cat -n validation/cross_validate_rust_python.py | sed -n '110,140p'Repository: ryancinsight/CFDrs
Length of output: 3243
Remove unnecessary f-string prefixes.
Lines 115 and 130 use f-strings without placeholders. Remove the f prefix from both lines.
🧹 Proposed fix
- print(f"\nRust implementation (via cfd_python.CarreauYasudaBlood):")
+ print("\nRust implementation (via cfd_python.CarreauYasudaBlood):")
cy_blood = cfd_python.CarreauYasudaBlood()
print(f"{'Shear Rate (s⁻¹)':>20} {'μ_rust (mPa·s)':>15} {'Diff %':>15}")
print("-" * 55)
all_match = True
for gamma_dot in test_shear_rates:
mu_python = carreau_yasuda_python(gamma_dot)
mu_rust = cy_blood.viscosity(gamma_dot)
diff_pct = abs(mu_rust - mu_python) / mu_python * 100
print(f"{gamma_dot:20.0f} {mu_rust * 1000:15.4f} {diff_pct:14.4e}%")
if diff_pct > 1e-4:
all_match = False
- print(f"\nComparison:")
+ print("\nComparison:")📝 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"\nRust implementation (via cfd_python.CarreauYasudaBlood):") | |
| cy_blood = cfd_python.CarreauYasudaBlood() | |
| # Try to test if we can create a blood model | |
| # Note: This depends on cfd_python API structure | |
| print(f"\n TODO: Add cfd_python API test if blood model is exposed") | |
| print(f"{'Shear Rate (s⁻¹)':>20} {'μ_rust (mPa·s)':>15} {'Diff %':>15}") | |
| print("-" * 55) | |
| all_match = True | |
| for gamma_dot in test_shear_rates: | |
| mu_python = carreau_yasuda_python(gamma_dot) | |
| mu_rust = cy_blood.viscosity(gamma_dot) | |
| diff_pct = abs(mu_rust - mu_python) / mu_python * 100 | |
| print(f"{gamma_dot:20.0f} {mu_rust * 1000:15.4f} {diff_pct:14.4e}%") | |
| if diff_pct > 1e-4: | |
| all_match = False | |
| print(f"\nComparison:") | |
| if all_match: | |
| print(" ✓ MATCH") | |
| else: | |
| print(" ✗ MISMATCH") | |
| print("\nRust implementation (via cfd_python.CarreauYasudaBlood):") | |
| cy_blood = cfd_python.CarreauYasudaBlood() | |
| print(f"{'Shear Rate (s⁻¹)':>20} {'μ_rust (mPa·s)':>15} {'Diff %':>15}") | |
| print("-" * 55) | |
| all_match = True | |
| for gamma_dot in test_shear_rates: | |
| mu_python = carreau_yasuda_python(gamma_dot) | |
| mu_rust = cy_blood.viscosity(gamma_dot) | |
| diff_pct = abs(mu_rust - mu_python) / mu_python * 100 | |
| print(f"{gamma_dot:20.0f} {mu_rust * 1000:15.4f} {diff_pct:14.4e}%") | |
| if diff_pct > 1e-4: | |
| all_match = False | |
| print("\nComparison:") | |
| if all_match: | |
| print(" ✓ MATCH") | |
| else: | |
| print(" ✗ MISMATCH") |
🧰 Tools
🪛 Ruff (0.15.4)
[error] 115-115: f-string without any placeholders
Remove extraneous f prefix
(F541)
[error] 130-130: 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` around lines 115 - 134, The two
print statements using unnecessary f-strings should be changed to plain string
literals: remove the leading `f` from the print that prints the Rust
implementation header (the print currently before creating `cy_blood =
cfd_python.CarreauYasudaBlood()`) and from the print that outputs the comparison
header (the print before checking `all_match`); keep all other prints (those
that format variables like `gamma_dot`, `mu_rust`, and `diff_pct`) as-is so
`test_shear_rates`, `carreau_yasuda_python`, and `cy_blood.viscosity` usage
remains unchanged.
| print(f"\nRust implementation (via cfd_python.GiersiepenModel):") | ||
| hemolysis_model = cfd_python.GiersiepenModel() | ||
|
|
||
| print(f"{'Stress (Pa)':>12} {'Time (s)':>12} {'Damage_rust':>15} {'Diff %':>15}") | ||
| print("-" * 55) | ||
|
|
||
| all_match = True | ||
| for tau, t in test_cases: | ||
| damage_python = giersiepen_python(tau, t) | ||
| damage_rust = hemolysis_model.calculate_damage(tau, t) | ||
| diff_pct = abs(damage_rust - damage_python) / damage_python * 100 | ||
| print(f"{tau:12.1f} {t:12.2f} {damage_rust:15.6f} {diff_pct:14.4e}%") | ||
|
|
||
| if diff_pct > 1e-4: | ||
| all_match = False | ||
|
|
||
| print(f"\nComparison:") | ||
| if all_match: | ||
| print(" ✓ MATCH") | ||
| else: | ||
| print(" ✗ MISMATCH") |
There was a problem hiding this comment.
Remove unnecessary f-string prefixes.
Lines 172 and 188 use f-strings without placeholders.
🧹 Proposed fix
- print(f"\nRust implementation (via cfd_python.GiersiepenModel):")
+ print("\nRust implementation (via cfd_python.GiersiepenModel):")
hemolysis_model = cfd_python.GiersiepenModel()
print(f"{'Stress (Pa)':>12} {'Time (s)':>12} {'Damage_rust':>15} {'Diff %':>15}")
print("-" * 55)
all_match = True
for tau, t in test_cases:
damage_python = giersiepen_python(tau, t)
damage_rust = hemolysis_model.calculate_damage(tau, t)
diff_pct = abs(damage_rust - damage_python) / damage_python * 100
print(f"{tau:12.1f} {t:12.2f} {damage_rust:15.6f} {diff_pct:14.4e}%")
if diff_pct > 1e-4:
all_match = False
- print(f"\nComparison:")
+ print("\nComparison:")📝 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"\nRust implementation (via cfd_python.GiersiepenModel):") | |
| hemolysis_model = cfd_python.GiersiepenModel() | |
| print(f"{'Stress (Pa)':>12} {'Time (s)':>12} {'Damage_rust':>15} {'Diff %':>15}") | |
| print("-" * 55) | |
| all_match = True | |
| for tau, t in test_cases: | |
| damage_python = giersiepen_python(tau, t) | |
| damage_rust = hemolysis_model.calculate_damage(tau, t) | |
| diff_pct = abs(damage_rust - damage_python) / damage_python * 100 | |
| print(f"{tau:12.1f} {t:12.2f} {damage_rust:15.6f} {diff_pct:14.4e}%") | |
| if diff_pct > 1e-4: | |
| all_match = False | |
| print(f"\nComparison:") | |
| if all_match: | |
| print(" ✓ MATCH") | |
| else: | |
| print(" ✗ MISMATCH") | |
| print("\nRust implementation (via cfd_python.GiersiepenModel):") | |
| hemolysis_model = cfd_python.GiersiepenModel() | |
| print(f"{'Stress (Pa)':>12} {'Time (s)':>12} {'Damage_rust':>15} {'Diff %':>15}") | |
| print("-" * 55) | |
| all_match = True | |
| for tau, t in test_cases: | |
| damage_python = giersiepen_python(tau, t) | |
| damage_rust = hemolysis_model.calculate_damage(tau, t) | |
| diff_pct = abs(damage_rust - damage_python) / damage_python * 100 | |
| print(f"{tau:12.1f} {t:12.2f} {damage_rust:15.6f} {diff_pct:14.4e}%") | |
| if diff_pct > 1e-4: | |
| all_match = False | |
| print("\nComparison:") | |
| if all_match: | |
| print(" ✓ MATCH") | |
| else: | |
| print(" ✗ MISMATCH") |
🧰 Tools
🪛 Ruff (0.15.4)
[error] 172-172: f-string without any placeholders
Remove extraneous f prefix
(F541)
[error] 188-188: 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` around lines 172 - 192, Two print
statements use f-strings without placeholders — remove the unnecessary 'f'
prefix to avoid misleading code and tiny overhead: change the print(f"\nRust
implementation (via cfd_python.GiersiepenModel):") and print(f"\nComparison:")
to plain print(...) calls; keep all other prints that use formatting (like the
formatted table and diff calculations using giersiepen_python and
hemolysis_model.calculate_damage) unchanged.
1 participant
This PR implements the requested
cfd_pythonbindings for Blake Threshold and Giersiepen Hemolysis. PyClass implementation handles data conversion correctly, and we removeTODOtags in the validation python script to assert that variables from both implementations match.PR created automatically by Jules for task 13880426803342757695 started by @ryancinsight
Summary by CodeRabbit
New Features
Chores
High-level PR Summary
This PR adds Python bindings for Blake Threshold cavitation calculations and Giersiepen hemolysis modeling to the
cfd_pythonpackage. Two new PyO3 wrapper classes (PyBlakeThresholdandPyGiersiepenModel) expose the existing Rust physics implementations to Python. The validation script is updated to remove TODO comments and now actively cross-validates the Rust implementations against Python reference calculations, confirming that both implementations produce matching results. Additionally, several import paths are updated to reflect refactored module structures in the underlying Rust crates.⏱️ Estimated Review Time: 15-30 minutes
💡 Review Order Suggestion
crates/cfd-python/src/lib.rscrates/cfd-python/src/cavitation.rscrates/cfd-python/src/hemolysis.rsvalidation/cross_validate_rust_python.pycrates/cfd-python/src/bifurcation.rscrates/cfd-python/src/womersley.rscrates/cfd-python/src/solver_2d/serpentine.rscrates/cfd-python/src/solver_2d/venturi.rs