feat: add python bindings for cavitation and hemolysis physics#254
feat: add python bindings for cavitation and hemolysis physics#254ryancinsight wants to merge 3 commits intomainfrom
Conversation
- Implemented `PyRayleighPlesset`, `PyCavitationRegimeClassifier`, and `PyCavitationRegimeAnalysis` in `cfd-python`. - Implemented `PyHemolysisModel` and `PyHemolysisCalculator` in `cfd-python`. - Added `build()` alias to `BranchingMeshBuilder`, `SerpentineMeshBuilder`, and `VenturiMeshBuilder` in `cfd-mesh` to resolve API compatibility with `cfd-3d`. - Updated `validation/cross_validate_rust_python.py` to cross-validate Rust implementations of Blake Threshold and Giersiepen Hemolysis against Python reference implementations. - Commented out external `blue2mesh` dependency in `cfd-optim` to prevent build failures in CI environment. 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. |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds PyO3 Python bindings for cavitation and hemolysis, exposes short Changes
Sequence Diagram(s)sequenceDiagram
participant Validator as Python Validator Script
participant PyModule as pycfdrs / pycfdrs alias
participant PyO3 as PyO3 Bindings (cavitation/hemolysis)
participant Rust as Rust core (inner models)
Validator->>PyModule: import pycfdrs (fallback handling)
PyModule->>PyO3: expose classes (PyRayleighPlesset, PyHemolysisModel, ...)
Validator->>PyO3: construct models & classifiers
PyO3->>Rust: delegate calls to inner Rust types (compute thresholds, analyses)
Rust-->>PyO3: return numeric results or errors
PyO3-->>Validator: return Py objects / raise PyValueError
Validator->>Validator: compare Rust-backed results vs Python, print diffs
alt mismatch
Validator->>System: sys.exit(non-zero)
else match
Validator->>Validator: print success
end
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 |
Summary of ChangesHello @ryancinsight, 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 Python interoperability of the CFD suite by introducing Python bindings for key physics models, specifically Rayleigh-Plesset cavitation and Giersiepen hemolysis. This allows Python users to leverage these advanced simulation capabilities. Additionally, it addresses a compilation issue by standardizing mesh builder APIs and streamlines the project's dependencies through a comprehensive 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
|
![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 1acc6f6..55a3a6c
✨ No bugs found, your code is sparkling clean
✅ Files analyzed, no issues (10)
• .gitignore
• Cargo.lock
• crates/cfd-mesh/src/geometry/branching.rs
• crates/cfd-mesh/src/geometry/serpentine.rs
• crates/cfd-mesh/src/geometry/venturi.rs
• crates/cfd-optim/Cargo.toml
• crates/cfd-python/src/cavitation.rs
• crates/cfd-python/src/hemolysis.rs
• crates/cfd-python/src/lib.rs
• validation/cross_validate_rust_python.py
![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 makes several valuable contributions. It introduces Python bindings for the Rayleigh-Plesset cavitation and Giersiepen hemolysis physics models, which is a great step for enabling cross-language validation and easier use of these models. The updates to the cross_validate_rust_python.py script to actually perform these validations are excellent. Additionally, the PR resolves a compilation error by adding build() aliases to mesh builders and cleans up dependencies by removing a broken one. The code for the new bindings is well-structured and clear. I have a few suggestions to improve the Python API by adding __repr__ methods for better debuggability and to clarify the usability of the PyBloodTrauma struct.
| fn __str__(&self) -> String { | ||
| self.inner.to_string() | ||
| } |
There was a problem hiding this comment.
It's good practice in Python to also implement __repr__ for an unambiguous object representation, which is especially useful for debugging. The __str__ method is for a user-friendly display. You can add a __repr__ method alongside the existing __str__.
fn __str__(&self) -> String {
self.inner.to_string()
}
fn __repr__(&self) -> String {
format!(
"CavitationRegimeAnalysis(regime={:?}, blake_threshold={:.2e}, inertial_threshold={:.2e})",
self.inner.regime,
self.inner.blake_threshold,
self.inner.inertial_threshold
)
}| #[pyclass(name = "BloodTrauma")] | ||
| pub struct PyBloodTrauma { | ||
| inner: RustBloodTrauma, | ||
| } |
There was a problem hiding this comment.
The PyBloodTrauma struct is exposed to Python, but it seems there's no way to create an instance of it from Python code since it lacks a #[new] constructor and no other function in the bindings appears to return it. Was this intended to be an output-only type for a function that is not yet implemented? If so, it might be better to either add the function that produces it or hold off on exposing this struct until it's usable.
| fn __repr__(&self) -> String { | ||
| self.__str__() | ||
| } |
There was a problem hiding this comment.
The __repr__ method should ideally return an unambiguous string representation of the object. Aliasing __str__, which provides a multi-line formatted output, is not standard practice for __repr__. A more conventional __repr__ would be more helpful for debugging.
fn __repr__(&self) -> String {
format!(
"BloodTrauma(hemolysis_level={:.2}, platelet_activation={:.2})",
self.inner.hemolysis_level, self.inner.platelet_activation
)
}![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 (6)
crates/cfd-python/src/hemolysis.rs (1)
12-14: Considerpub(crate)instead ofpubforinner.
inneris public so thatPyHemolysisCalculator::new(line 77) can accessmodel.inner.clone(). Since that's an intra-crate access,pub(crate)would restrict the field from leaking outside the crate boundary while still allowing internal use.Proposed fix
pub struct PyHemolysisModel { - pub inner: RustHemolysisModel, + pub(crate) inner: RustHemolysisModel, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cfd-python/src/hemolysis.rs` around lines 12 - 14, Change the visibility of the PyHemolysisModel.inner field from pub to pub(crate) so it remains accessible to PyHemolysisCalculator::new and other intra-crate code but is not exposed outside the crate; locate the struct PyHemolysisModel and update the inner field's visibility accordingly, then run cargo build/tests to ensure no external API consumers require the field to be public.validation/cross_validate_rust_python.py (1)
93-95: Missing zero-division guard, unlike the analogous checks on lines 158 and 219.Lines 158 and 219 guard against division by zero (
if mu_python > 0,if damage_python > 0), but lines 94–95 do not. While the physical constants guarantee nonzero values here, adding the same guard would be consistent.Proposed fix for consistency
- diff_Rc = abs(R_c_rust - R_c_python) / R_c_python * 100 - diff_P = abs(P_Blake_rust - P_Blake_python) / P_Blake_python * 100 + diff_Rc = abs(R_c_rust - R_c_python) / R_c_python * 100 if R_c_python > 0 else 0 + diff_P = abs(P_Blake_rust - P_Blake_python) / P_Blake_python * 100 if P_Blake_python > 0 else 0🤖 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 93 - 95, The percent-difference calculations for diff_Rc and diff_P lack zero-division guards; update the block where diff_Rc and diff_P are computed so you only perform division when R_c_python > 0 and P_Blake_python > 0 (mirror the existing patterns used for mu_python and damage_python checks), otherwise set the diffs to 0 or an appropriate sentinel; locate the expressions computing diff_Rc (R_c_rust, R_c_python) and diff_P (P_Blake_rust, P_Blake_python) and apply the same guard logic.crates/cfd-optim/Cargo.toml (1)
14-14: Commented-out code instead of removal — acceptable as a temporary measure.Both the feature and its dependency are consistently disabled. Consider adding a TODO comment (e.g.,
# TODO: re-enable when blue2mesh build is fixed) or tracking this in an issue so it doesn't rot silently.Also applies to: 24-24
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cfd-optim/Cargo.toml` at line 14, The commented-out dependency line '# mesh-export = ["dep:blue2mesh"]' should not be left undocumented; replace or annotate the commented line with a short TODO explaining why it was disabled and reference an issue or tracker ID (e.g., "TODO: re-enable when blue2mesh build is fixed — see ISSUE-123") so the disabling doesn't rot silently, and do the same for the other occurrence of the commented mesh-export line.crates/cfd-python/src/cavitation.rs (3)
14-15: Tighteninnervisibility frompubtopub(crate).
PyRayleighPlesset.innerispub, which exposesRustRayleighPlesset<f64>as part of the public Rust API. Intra-crate access (used inPyCavitationRegimeClassifier::newat line 85) only requirespub(crate). Theinnerfields onPyCavitationRegimeClassifierandPyCavitationRegimeAnalysisare already private, so this inconsistency is likely unintentional.♻️ Proposed fix
pub struct PyRayleighPlesset { - pub inner: RustRayleighPlesset<f64>, + pub(crate) inner: RustRayleighPlesset<f64>, }🤖 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 14 - 15, Change the visibility of the PyRayleighPlesset.inner field from pub to pub(crate) so the RustRayleighPlesset<f64> type is not exposed in the public API; update the struct declaration for PyRayleighPlesset (the inner field) to pub(crate) and ensure any intra-crate users such as PyCavitationRegimeClassifier::new and related code that accesses inner continue to compile; no other public-facing structs (e.g., PyCavitationRegimeAnalysis) need changes.
171-173: Add__repr__alongside__str__onPyCavitationRegimeAnalysis.Having
__str__without__repr__meansrepr(analysis)falls back to the default PyO3 object representation, which is uninformative during debugging. Delegating__repr__to the sameto_string()(or a dedicated method) is idiomatic.♻️ Proposed fix
fn __str__(&self) -> String { self.inner.to_string() } + + fn __repr__(&self) -> String { + self.inner.to_string() + }🤖 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 171 - 173, Add a __repr__ method to PyCavitationRegimeAnalysis that mirrors the existing __str__ implementation so repr(analysis) returns the same human-readable string; locate the impl block where fn __str__(&self) -> String delegates to self.inner.to_string() and add fn __repr__(&self) -> String { self.inner.to_string() } (or call the same helper/to_string method) so both __str__ and __repr__ use inner.to_string().
20-39: No validation of physical parameters — consider guarding against non-physical inputs.
PyRayleighPlesset::newdirectly sets all fields without any range checks. Values likeinitial_radius <= 0,liquid_density <= 0, orliquid_viscosity < 0are physically meaningless and would produce incorrect orNaN/infresults from downstream physics calculations (e.g.blake_critical_radius). Additionally, directly constructingRustRayleighPlesset { ... }bypasses any validation logic that may exist in its own constructor — prefer callingRustRayleighPlesset::new(...)if one exists.🛡️ Proposed fix (direct guard approach)
+use pyo3::exceptions::PyValueError; #[pymethods] impl PyRayleighPlesset { #[new] fn new( initial_radius: f64, liquid_density: f64, liquid_viscosity: f64, surface_tension: f64, vapor_pressure: f64, polytropic_index: f64, - ) -> Self { - PyRayleighPlesset { + ) -> PyResult<Self> { + if initial_radius <= 0.0 || liquid_density <= 0.0 || liquid_viscosity < 0.0 || polytropic_index <= 0.0 { + return Err(PyValueError::new_err("Physical parameters must be positive (initial_radius, liquid_density, polytropic_index > 0; liquid_viscosity >= 0)")); + } + Ok(PyRayleighPlesset { inner: RustRayleighPlesset { initial_radius, liquid_density, liquid_viscosity, surface_tension, vapor_pressure, polytropic_index, }, - } + }) }🤖 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 20 - 39, PyRayleighPlesset::new must validate physical inputs and avoid directly constructing RustRayleighPlesset with invalid values; change the #[new] constructor to return PyResult<Self>, check that initial_radius > 0, liquid_density > 0, liquid_viscosity >= 0, surface_tension >= 0, vapor_pressure >= 0 and a sensible polytropic_index (e.g. > 0), and if invalid return a PyValueError; after validation call RustRayleighPlesset::new(...) if that ctor exists (or construct the inner struct only after validation) so downstream methods like blake_critical_radius get valid state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/cfd-python/src/cavitation.rs`:
- Around line 50-55: The enum variant PyCavitationRegime::None is inaccessible
from Python because it becomes CavitationRegime.None (a Python keyword); rename
the variant to a non-keyword identifier (e.g., PyCavitationRegime::NoCavitation
or ::Absent) and update all usages accordingly, including the
From<RustCavitationRegime> implementation to map the original
RustCavitationRegime::None case to the new PyCavitationRegime variant and adjust
any match arms, tests, and documentation that reference
PyCavitationRegime::None.
In `@validation/cross_validate_rust_python.py`:
- Around line 259-265: The print call that begins with print(f"""Python
implementations validated against literature...""") uses an unnecessary f-string
prefix (Ruff F541); remove the leading "f" so the multiline string is a normal
triple-quoted string. Locate the print(...) block in
validation/cross_validate_rust_python.py (the multiline message about completing
Rust validation and maturin develop) and change print(f"""...""") to
print("""...""") to resolve the lint error.
- Around line 207-208: Remove the unnecessary f-string prefixes flagged by Ruff
F541 on the two plain-print statements: change the calls to print(f" Located
in: crates/cfd-core/src/physics/hemolysis.rs") and print(f" Method:
damage_index(shear_stress, exposure_time)") to use normal string literals
(remove the leading "f") so they become print(" Located in:
crates/cfd-core/src/physics/hemolysis.rs") and print(" Method:
damage_index(shear_stress, exposure_time)"); this fixes the linter warning
without changing output.
---
Nitpick comments:
In `@crates/cfd-optim/Cargo.toml`:
- Line 14: The commented-out dependency line '# mesh-export = ["dep:blue2mesh"]'
should not be left undocumented; replace or annotate the commented line with a
short TODO explaining why it was disabled and reference an issue or tracker ID
(e.g., "TODO: re-enable when blue2mesh build is fixed — see ISSUE-123") so the
disabling doesn't rot silently, and do the same for the other occurrence of the
commented mesh-export line.
In `@crates/cfd-python/src/cavitation.rs`:
- Around line 14-15: Change the visibility of the PyRayleighPlesset.inner field
from pub to pub(crate) so the RustRayleighPlesset<f64> type is not exposed in
the public API; update the struct declaration for PyRayleighPlesset (the inner
field) to pub(crate) and ensure any intra-crate users such as
PyCavitationRegimeClassifier::new and related code that accesses inner continue
to compile; no other public-facing structs (e.g., PyCavitationRegimeAnalysis)
need changes.
- Around line 171-173: Add a __repr__ method to PyCavitationRegimeAnalysis that
mirrors the existing __str__ implementation so repr(analysis) returns the same
human-readable string; locate the impl block where fn __str__(&self) -> String
delegates to self.inner.to_string() and add fn __repr__(&self) -> String {
self.inner.to_string() } (or call the same helper/to_string method) so both
__str__ and __repr__ use inner.to_string().
- Around line 20-39: PyRayleighPlesset::new must validate physical inputs and
avoid directly constructing RustRayleighPlesset with invalid values; change the
#[new] constructor to return PyResult<Self>, check that initial_radius > 0,
liquid_density > 0, liquid_viscosity >= 0, surface_tension >= 0, vapor_pressure
>= 0 and a sensible polytropic_index (e.g. > 0), and if invalid return a
PyValueError; after validation call RustRayleighPlesset::new(...) if that ctor
exists (or construct the inner struct only after validation) so downstream
methods like blake_critical_radius get valid state.
In `@crates/cfd-python/src/hemolysis.rs`:
- Around line 12-14: Change the visibility of the PyHemolysisModel.inner field
from pub to pub(crate) so it remains accessible to PyHemolysisCalculator::new
and other intra-crate code but is not exposed outside the crate; locate the
struct PyHemolysisModel and update the inner field's visibility accordingly,
then run cargo build/tests to ensure no external API consumers require the field
to be public.
In `@validation/cross_validate_rust_python.py`:
- Around line 93-95: The percent-difference calculations for diff_Rc and diff_P
lack zero-division guards; update the block where diff_Rc and diff_P are
computed so you only perform division when R_c_python > 0 and P_Blake_python > 0
(mirror the existing patterns used for mu_python and damage_python checks),
otherwise set the diffs to 0 or an appropriate sentinel; locate the expressions
computing diff_Rc (R_c_rust, R_c_python) and diff_P (P_Blake_rust,
P_Blake_python) and apply the same guard logic.
| pub enum PyCavitationRegime { | ||
| None, | ||
| Stable, | ||
| Inertial, | ||
| Mixed, | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Python 3 attribute access with None keyword obj.None SyntaxError
💡 Result:
obj.None is a SyntaxError because None is a keyword token in Python, and the dotted attribute syntax requires an identifier (name) after the dot—keywords are not valid identifiers in that position, so the code is rejected by the parser before any attribute lookup happens. [1] [2]
If you really have an attribute whose name is the string "None" (e.g., set dynamically), you must access it by string name:
setattr(obj, "None", 123)
getattr(obj, "None") # 123
obj.__dict__["None"] # 123 (for normal instances)To prevent creating attributes that can’t be accessed with obj.name, validate names like this:
import keyword
def ok_attr_name(s: str) -> bool:
return isinstance(s, str) and s.isidentifier() and not keyword.iskeyword(s)[3]
If your real issue was “obj is None”, then you’d get AttributeError: 'NoneType' object has no attribute ... (not a SyntaxError).
🏁 Script executed:
# Check if the file exists and examine the relevant code
find . -name "cavitation.rs" -type f | head -20Repository: ryancinsight/CFDrs
Length of output: 99
🏁 Script executed:
# Once file is found, examine the code around line 50 and line 85
fd cavitation.rs -x cat -n {} \; | head -150Repository: ryancinsight/CFDrs
Length of output: 5189
🏁 Script executed:
# Search for RustRayleighPlesset definition to check Copy/Clone traits
rg "struct RustRayleighPlesset" -A 5Repository: ryancinsight/CFDrs
Length of output: 44
🏁 Script executed:
# Search across entire codebase for RustRayleighPlesset definition
rg "struct RustRayleighPlesset|pub struct RustRayleighPlesset" -A 10Repository: ryancinsight/CFDrs
Length of output: 44
🏁 Script executed:
# Also check if there's a Clone or Copy derive on the type
rg "RayleighPlesset" -B 2 -A 3 | head -100Repository: ryancinsight/CFDrs
Length of output: 7291
🏁 Script executed:
# Search in cfd-core crate for RayleighPlesset struct definition
find crates/cfd-core -name "*.rs" -type f | xargs rg "struct RayleighPlesset" -B 2 -A 8Repository: ryancinsight/CFDrs
Length of output: 1101
🏁 Script executed:
# Also check if cfd-core defines Copy trait on RayleighPlesset
find crates/cfd-core -name "*.rs" -type f | xargs rg "RayleighPlesset" -B 3 -A 3 | head -80Repository: ryancinsight/CFDrs
Length of output: 5737
CavitationRegime.None is inaccessible from Python — rename this variant.
PyO3 adds a class attribute for each unit variant, so users access them as e.g. CavitationRegime.Variant. The variant None (line 51) will be exposed as CavitationRegime.None. In Python 3, None is a reserved keyword, so CavitationRegime.None is a hard SyntaxError at parse time — the variant is completely unreachable through normal attribute access. Users would need the awkward getattr(CavitationRegime, "None") workaround.
Rename the variant to something like NoCavitation or Absent and update the From<RustCavitationRegime> impl accordingly:
🐛 Proposed fix
pub enum PyCavitationRegime {
- None,
+ NoCavitation,
Stable,
Inertial,
Mixed,
}
impl From<RustCavitationRegime> for PyCavitationRegime {
fn from(regime: RustCavitationRegime) -> Self {
match regime {
- RustCavitationRegime::None => PyCavitationRegime::None,
+ RustCavitationRegime::None => PyCavitationRegime::NoCavitation,
RustCavitationRegime::Stable => PyCavitationRegime::Stable,
RustCavitationRegime::Inertial => PyCavitationRegime::Inertial,
RustCavitationRegime::Mixed => PyCavitationRegime::Mixed,
}
}
}🤖 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 50 - 55, The enum variant
PyCavitationRegime::None is inaccessible from Python because it becomes
CavitationRegime.None (a Python keyword); rename the variant to a non-keyword
identifier (e.g., PyCavitationRegime::NoCavitation or ::Absent) and update all
usages accordingly, including the From<RustCavitationRegime> implementation to
map the original RustCavitationRegime::None case to the new PyCavitationRegime
variant and adjust any match arms, tests, and documentation that reference
PyCavitationRegime::None.
| print(f" Located in: crates/cfd-core/src/physics/hemolysis.rs") | ||
| print(f" Method: damage_index(shear_stress, exposure_time)") |
There was a problem hiding this comment.
Remove extraneous f prefix on plain strings (flagged by Ruff F541).
These strings contain no placeholders, so the f prefix is unnecessary.
Proposed fix
- print(f" Located in: crates/cfd-core/src/physics/hemolysis.rs")
- print(f" Method: damage_index(shear_stress, exposure_time)")
+ print(" Located in: crates/cfd-core/src/physics/hemolysis.rs")
+ print(" Method: damage_index(shear_stress, exposure_time)")📝 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" Located in: crates/cfd-core/src/physics/hemolysis.rs") | |
| print(f" Method: damage_index(shear_stress, exposure_time)") | |
| print(" Located in: crates/cfd-core/src/physics/hemolysis.rs") | |
| print(" Method: damage_index(shear_stress, exposure_time)") |
🧰 Tools
🪛 Ruff (0.15.1)
[error] 207-207: f-string without any placeholders
Remove extraneous f prefix
(F541)
[error] 208-208: 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 207 - 208, Remove the
unnecessary f-string prefixes flagged by Ruff F541 on the two plain-print
statements: change the calls to print(f" Located in:
crates/cfd-core/src/physics/hemolysis.rs") and print(f" Method:
damage_index(shear_stress, exposure_time)") to use normal string literals
(remove the leading "f") so they become print(" Located in:
crates/cfd-core/src/physics/hemolysis.rs") and print(" Method:
damage_index(shear_stress, exposure_time)"); this fixes the linter warning
without changing output.
| print(f""" | ||
| Python implementations validated against literature. | ||
|
|
||
| To complete Rust validation: | ||
| 1. Build pycfdrs: maturin develop --release | ||
| 2. Expose required methods in Python bindings | ||
| 3. Re-run this script to cross-check values | ||
|
|
||
| Rust files to verify: | ||
| - crates/cfd-core/src/physics/cavitation/regimes.rs (Blake threshold) | ||
| - crates/cfd-core/src/physics/fluid/blood.rs (Carreau-Yasuda) | ||
| - crates/cfd-core/src/physics/hemolysis/giersiepen.rs (Giersiepen model) | ||
| 2. Re-run this script to cross-check values | ||
| """) |
There was a problem hiding this comment.
Another extraneous f prefix (Ruff F541).
Proposed fix
- print(f"""
+ print("""
Python implementations validated against literature.
To complete Rust validation:
1. Build pycfdrs: maturin develop --release
2. Re-run this script to cross-check values
""")🧰 Tools
🪛 Ruff (0.15.1)
[error] 259-265: 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 259 - 265, The print
call that begins with print(f"""Python implementations validated against
literature...""") uses an unnecessary f-string prefix (Ruff F541); remove the
leading "f" so the multiline string is a normal triple-quoted string. Locate the
print(...) block in validation/cross_validate_rust_python.py (the multiline
message about completing Rust validation and maturin develop) and change
print(f"""...""") to print("""...""") to resolve the lint error.
- Implemented `PyRayleighPlesset`, `PyCavitationRegimeClassifier`, and `PyCavitationRegimeAnalysis` in `cfd-python`. - Implemented `PyHemolysisModel` and `PyHemolysisCalculator` in `cfd-python`. - Updated `validation/cross_validate_rust_python.py` to cross-validate Rust implementations of Blake Threshold and Giersiepen Hemolysis against Python reference implementations. - Added `build()` alias to `BranchingMeshBuilder`, `SerpentineMeshBuilder`, and `VenturiMeshBuilder` in `cfd-mesh` to resolve API compatibility with `cfd-3d`. - Commented out `blue2mesh` dependency in `cfd-optim` to prevent build failures. - Fixed CI build failure by installing `libfontconfig1-dev` in `performance-benchmarking.yml`. Co-authored-by: ryancinsight <55164720+ryancinsight@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
crates/cfd-python/src/hemolysis.rs (2)
12-13: Preferpub(crate)overpubfor theinnerfield.
pub innerexposes the rawRustHemolysisModeloutside the crate. The only consumer isPyHemolysisCalculator::new(line 77), which is within the same crate, sopub(crate)is sufficient and avoids leaking the Rust-internal type through the public API surface.♻️ Proposed visibility fix
pub struct PyHemolysisModel { - pub inner: RustHemolysisModel, + pub(crate) inner: RustHemolysisModel, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cfd-python/src/hemolysis.rs` around lines 12 - 13, The field visibility for PyHemolysisModel currently exposes the internal RustHemolysisModel as pub; change the field declaration to pub(crate) so the raw RustHemolysisModel is not leaked in the public API. Update the declaration of PyHemolysisModel::inner to pub(crate) and confirm usages (e.g., in PyHemolysisCalculator::new) still compile within the crate.
168-174:__repr__delegates to__str__, violating Python convention.Python's data model reserves
__repr__for a developer-facing, unambiguous representation (ideally reconstructable), while__str__is for end-user display. Delegating__repr__to__str__collapses the distinction and can mislead debuggers/REPLs. At minimum, callself.inner.to_string()directly to remove the indirect call; ideally,__repr__should include the class name and field values.♻️ Minimal fix (removes indirection)
fn __repr__(&self) -> String { - self.__str__() + format!("BloodTrauma({})", self.inner) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cfd-python/src/hemolysis.rs` around lines 168 - 174, The current __repr__ delegates to __str__ which violates Python conventions; update the __repr__ implementation in hemolysis.rs to not call self.__str__() but instead return a developer-facing representation such as format!("Hemolysis({})", self.inner.to_string()) (or at minimum call self.inner.to_string() directly); modify the method named __repr__ to build a string including the type name and inner field value rather than delegating to __str__.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/cfd-python/src/hemolysis.rs`:
- Around line 107-114: The enum PyBloodTraumaSeverity currently provides
equality but is unhashable at runtime; update its definition to derive Hash
(i.e., #[derive(Clone, Copy, PartialEq, Eq, Hash)]) and enable hashing in the
pyo3 attribute by changing #[pyclass(name = "BloodTraumaSeverity", eq)] to
include hash and frozen (e.g., #[pyclass(name = "BloodTraumaSeverity", eq, hash,
frozen)]) so Python sees a proper __hash__ along with __eq__; adjust the
derive/attribute on PyBloodTraumaSeverity accordingly.
- Around line 127-131: PyBloodTrauma is exported but has no constructor or
conversion from RustBloodTrauma, so Python cannot create instances; add a public
constructor annotated #[new] on an impl block for PyBloodTrauma that accepts
appropriate fields or a RustBloodTrauma and initializes PyBloodTrauma { inner:
RustBloodTrauma }, and/or implement From<RustBloodTrauma> for PyBloodTrauma (and
optionally IntoPy<PyObject> if used) so Rust code can return PyBloodTrauma to
Python; update the impl block for PyBloodTrauma and ensure the pyclass is
exposed in the module initialization.
---
Nitpick comments:
In `@crates/cfd-python/src/hemolysis.rs`:
- Around line 12-13: The field visibility for PyHemolysisModel currently exposes
the internal RustHemolysisModel as pub; change the field declaration to
pub(crate) so the raw RustHemolysisModel is not leaked in the public API. Update
the declaration of PyHemolysisModel::inner to pub(crate) and confirm usages
(e.g., in PyHemolysisCalculator::new) still compile within the crate.
- Around line 168-174: The current __repr__ delegates to __str__ which violates
Python conventions; update the __repr__ implementation in hemolysis.rs to not
call self.__str__() but instead return a developer-facing representation such as
format!("Hemolysis({})", self.inner.to_string()) (or at minimum call
self.inner.to_string() directly); modify the method named __repr__ to build a
string including the type name and inner field value rather than delegating to
__str__.
| #[pyclass(name = "BloodTraumaSeverity", eq)] | ||
| #[derive(Clone, Copy, PartialEq, Eq)] | ||
| pub enum PyBloodTraumaSeverity { | ||
| Minimal, | ||
| Moderate, | ||
| Severe, | ||
| Critical, | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the actual code in the repository
cat -n crates/cfd-python/src/hemolysis.rs | sed -n '100,120p'Repository: ryancinsight/CFDrs
Length of output: 887
🌐 Web query:
PyO3 pyclass eq hash frozen implementation Python hashable enum
💡 Result:
To make a PyO3 #[pyclass] type hashable in Python (usable as a dict key / in a set), use PyO3’s built-in implementations:
#[pyclass(eq)]→ generates__eq__from RustPartialEq#[pyclass(frozen)]→ marks the object immutable (required for safe hashing)#[pyclass(hash)]→ generates__hash__from RustHash(requireseqandfrozen) [1]
Example (struct)
use pyo3::prelude::*;
use std::hash::{Hash};
#[pyclass(eq, frozen, hash)]
#[derive(Clone, PartialEq, Eq, Hash)]
pub struct Key {
#[pyo3(get)]
a: u64,
#[pyo3(get)]
b: String,
}Example (fieldless enum)
PyO3 supports exposing fieldless enums as Python objects via #[pyclass]; the same eq/frozen/hash pattern applies if you derive the Rust traits [1].
use pyo3::prelude::*;
use std::hash::Hash;
#[pyclass(eq, frozen, hash)]
#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)]
pub enum Color {
Red,
Green,
Blue,
}If you don’t want it hashable
Set __hash__ = None (Python’s standard way to make a type unhashable) [2]:
#[pyclass]
pub struct NotHashable;
#[pymethods]
impl NotHashable {
#[classattr]
const __hash__: Option<Py<PyAny>> = None;
}Notes
- If you can’t (or don’t want to) use
#[pyclass(eq, frozen, hash)], you can still implement comparisons manually with__richcmp__, but theeq/hash/frozenparameters are the simplest “Python-hashable” route when your Rust type can derivePartialEq/Eq/Hash. [2]
Sources: PyO3 #[pyclass] parameters (eq, frozen, hash) [1]; PyO3 guide on __hash__ = None and rich comparisons [2].
PyBloodTraumaSeverity will be unhashable in Python at runtime.
Using #[pyclass(eq)] generates __eq__ and silently suppresses Python's default __hash__. Any attempt to use BloodTraumaSeverity as a dict key or in a set will raise TypeError: unhashable type: 'BloodTraumaSeverity'.
Since this enum derives Clone + Copy + PartialEq + Eq, add Hash to the derive list and enable hash (with frozen) in the #[pyclass] attribute. PyO3's hash option requires both eq and frozen.
🐛 Proposed fix
-#[pyclass(name = "BloodTraumaSeverity", eq)]
-#[derive(Clone, Copy, PartialEq, Eq)]
+#[pyclass(name = "BloodTraumaSeverity", eq, frozen, hash)]
+#[derive(Clone, Copy, PartialEq, Eq, Hash)]
pub enum PyBloodTraumaSeverity {📝 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.
| #[pyclass(name = "BloodTraumaSeverity", eq)] | |
| #[derive(Clone, Copy, PartialEq, Eq)] | |
| pub enum PyBloodTraumaSeverity { | |
| Minimal, | |
| Moderate, | |
| Severe, | |
| Critical, | |
| } | |
| #[pyclass(name = "BloodTraumaSeverity", eq, frozen, hash)] | |
| #[derive(Clone, Copy, PartialEq, Eq, Hash)] | |
| pub enum PyBloodTraumaSeverity { | |
| Minimal, | |
| Moderate, | |
| Severe, | |
| Critical, | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/cfd-python/src/hemolysis.rs` around lines 107 - 114, The enum
PyBloodTraumaSeverity currently provides equality but is unhashable at runtime;
update its definition to derive Hash (i.e., #[derive(Clone, Copy, PartialEq, Eq,
Hash)]) and enable hashing in the pyo3 attribute by changing #[pyclass(name =
"BloodTraumaSeverity", eq)] to include hash and frozen (e.g., #[pyclass(name =
"BloodTraumaSeverity", eq, hash, frozen)]) so Python sees a proper __hash__
along with __eq__; adjust the derive/attribute on PyBloodTraumaSeverity
accordingly.
| /// Blood trauma assessment | ||
| #[pyclass(name = "BloodTrauma")] | ||
| pub struct PyBloodTrauma { | ||
| inner: RustBloodTrauma, | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n --type rust "PyBloodTrauma" --context 3Repository: ryancinsight/CFDrs
Length of output: 3180
🏁 Script executed:
sed -n '133,200p' crates/cfd-python/src/hemolysis.rsRepository: ryancinsight/CFDrs
Length of output: 906
🏁 Script executed:
rg -n "-> RustBloodTrauma\|-> PyBloodTrauma" --type rust crates/cfd-python/src/hemolysis.rsRepository: ryancinsight/CFDrs
Length of output: 466
🏁 Script executed:
rg -n "-> RustBloodTrauma|-> PyBloodTrauma" --type rust crates/cfd-python/src/hemolysis.rsRepository: ryancinsight/CFDrs
Length of output: 466
🏁 Script executed:
rg -n "RustBloodTrauma" --type rust crates/cfd-python/src/hemolysis.rs | head -20Repository: ryancinsight/CFDrs
Length of output: 645
🏁 Script executed:
rg -n "PyHemolysisCalculator" --type rust -A 5 crates/cfd-python/src/hemolysis.rs | head -50Repository: ryancinsight/CFDrs
Length of output: 525
🏁 Script executed:
sed -n '62,105p' crates/cfd-python/src/hemolysis.rsRepository: ryancinsight/CFDrs
Length of output: 1327
🏁 Script executed:
rg -n "fn.*->.*BloodTrauma\|PyBloodTrauma\s*{" --type rust crates/cfd-python/src/hemolysis.rsRepository: ryancinsight/CFDrs
Length of output: 201
🏁 Script executed:
rg -n "fn.*PyBloodTrauma" --type rust crates/cfd-python/src/hemolysis.rsRepository: ryancinsight/CFDrs
Length of output: 115
🏁 Script executed:
wc -l crates/cfd-python/src/hemolysis.rsRepository: ryancinsight/CFDrs
Length of output: 100
🏁 Script executed:
cat -n crates/cfd-python/src/hemolysis.rsRepository: ryancinsight/CFDrs
Length of output: 5796
🏁 Script executed:
rg -n "PyBloodTrauma" crates/cfd-python/src/ --type rustRepository: ryancinsight/CFDrs
Length of output: 1074
🏁 Script executed:
rg -n "From.*RustBloodTrauma\>" crates/cfd-python/src/ --type rustRepository: ryancinsight/CFDrs
Length of output: 172
🏁 Script executed:
rg -n "From.*RustBloodTrauma" crates/cfd-python/src/ --type rustRepository: ryancinsight/CFDrs
Length of output: 163
PyBloodTrauma has no constructor and no visible factory — Python users cannot obtain an instance.
There is no #[new] method and no Rust function in the cfd-python crate that returns a PyBloodTrauma. Unlike PyBloodTraumaSeverity, which has a From<RustBloodTraumaSeverity> conversion, PyBloodTrauma has no such conversion mechanism. The class is exported to the Python module but remains inaccessible.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/cfd-python/src/hemolysis.rs` around lines 127 - 131, PyBloodTrauma is
exported but has no constructor or conversion from RustBloodTrauma, so Python
cannot create instances; add a public constructor annotated #[new] on an impl
block for PyBloodTrauma that accepts appropriate fields or a RustBloodTrauma and
initializes PyBloodTrauma { inner: RustBloodTrauma }, and/or implement
From<RustBloodTrauma> for PyBloodTrauma (and optionally IntoPy<PyObject> if
used) so Rust code can return PyBloodTrauma to Python; update the impl block for
PyBloodTrauma and ensure the pyclass is exposed in the module initialization.
- Fix type mismatch in `cfd-3d` solvers caused by `build()` returning `IndexedMesh` (surface) instead of `Mesh<T>` (volume). - Implement `impl<T: Scalar> From<IndexedMesh<T>> for Mesh<T>` in `crates/cfd-mesh/src/mesh.rs` to bridge the new surface mesh type to the legacy volume mesh type. - Add `Mesh::cast<U>` method to support type conversion during mesh building. - Update `BranchingMeshBuilder`, `SerpentineMeshBuilder`, and `VenturiMeshBuilder` `build()` methods to return `Mesh<T>` by converting via `f64` and casting. - Fix ambiguous `sqrt` and `abs` calls in `crates/cfd-3d/src/serpentine/validation.rs` by explicitly using `num_traits::Float`. - Add `libfontconfig1-dev` to GitHub Actions workflow to fix `plotters` build dependency failure on Linux. - Fix PyO3 deprecation warning by removing `eq_int` from `#[pyclass]` attributes. Co-authored-by: ryancinsight <55164720+ryancinsight@users.noreply.github.com>
1 participant
This PR implements Python bindings for critical physics models (Rayleigh-Plesset cavitation and Giersiepen hemolysis) as requested by TODO tags in the validation suite. It also resolves a compilation error in
cfd-3dby adding missingbuild()methods to mesh builders incfd-meshand disables a broken external dependency incfd-optim.Changes:
cavitation.rsandhemolysis.rsexposing core physics structs.build()method to builders as an alias forbuild_surface().cross_validate_rust_python.py.blue2meshdependency.PR created automatically by Jules for task 10596807140692364424 started by @ryancinsight
High-level PR Summary
This PR adds Python bindings for critical physics models (Rayleigh-Plesset cavitation and Giersiepen hemolysis) to enable cross-language validation, adds
build()method aliases to mesh builders incfd-meshto fix compilation errors incfd-3d, and disables the brokenblue2meshdependency incfd-optim. The validation suite is updated to test that Python bindings produce identical results to direct Python implementations.⏱️ Estimated Review Time: 30-90 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-mesh/src/geometry/branching.rscrates/cfd-mesh/src/geometry/serpentine.rscrates/cfd-mesh/src/geometry/venturi.rscrates/cfd-optim/Cargo.tomlCargo.lock.gitignoreSummary by CodeRabbit
New Features
API Improvements
Chores