Conversation
- Remove `pub mod api` and move re-exports to `cfd_core::physics` top-level. - Add missing `cavitation` re-exports. - Group re-exports by domain (Fluid, Boundary, Dynamics, Bio, etc.). - Update documentation for `cfd_core::physics`. - Add comment to `crates/cfd-optim/Cargo.toml` regarding disabled `blue2mesh` dependency. 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. |
📝 WalkthroughWalkthroughReorganizes the physics public API to use explicit top-level re-exports; disables mesh-export/blue2mesh in cfd-optim; adds volume (tetrahedral) mesh build paths to Venturi, Serpentine, and Branching mesh builders; tweaks numeric trait usage in serpentine validation; and adds libfontconfig1-dev to the benchmarking workflow runner. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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 focuses on enhancing 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
|
![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..190eecc
✨ No bugs found, your code is sparkling clean
✅ Files analyzed, no issues (3)
• Cargo.lock
• crates/cfd-core/src/physics/mod.rs
• crates/cfd-optim/Cargo.toml
![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 refactors the cfd-core::physics module by flattening the api submodule into direct re-exports at the top level. This change improves API ergonomics and clarity for consumers of the library. Additionally, the PR introduces missing cavitation types to the physics re-exports and disables the blue2mesh dependency in cfd-optim as per project instructions. The Cargo.lock updates correctly reflect the removal of transitive dependencies associated with the disabled feature.
| pub use cavitation::{ | ||
| CavitationDamage, CavitationModel, CavitationNumber, CavitationRegime, RayleighPlesset, | ||
| VenturiCavitation, | ||
| }; |
There was a problem hiding this comment.
The cavitation re-exports are missing CavitationRegimeAnalysis and CavitationRegimeClassifier. These types are essential for performing and interpreting cavitation regime analysis using the provided bubble models. Including them ensures the top-level physics API is complete and consistent with other submodules like fluid_dynamics and hemolysis which re-export their primary analysis tools.
| pub use cavitation::{ | |
| CavitationDamage, CavitationModel, CavitationNumber, CavitationRegime, RayleighPlesset, | |
| VenturiCavitation, | |
| }; | |
| pub use cavitation::{ | |
| CavitationDamage, CavitationModel, CavitationNumber, CavitationRegime, | |
| CavitationRegimeAnalysis, CavitationRegimeClassifier, RayleighPlesset, | |
| VenturiCavitation, | |
| }; |
![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
🧹 Nitpick comments (1)
crates/cfd-core/src/physics/mod.rs (1)
49-49:pub use constants::{mathematical, physics}creates the confusing pathcfd_core::physics::physics.Re-exporting the
constants::physicssub-module at the top of thephysicsmodule produces the redundant accessorcfd_core::physics::physics. Consider aliasing to avoid the doublephysicssegment:♻️ Proposed alias to disambiguate
-pub use constants::{mathematical, physics}; +pub use constants::mathematical; +pub use constants::physics as physics_constants;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cfd-core/src/physics/mod.rs` at line 49, The re-export `pub use constants::{mathematical, physics};` creates the redundant path `cfd_core::physics::physics`; change the re-export to alias the inner `physics` module (for example `pub use constants::{mathematical, physics as physics_constants};`) so callers use `cfd_core::physics::physics_constants` (or another clear alias you choose) and update any downstream references to the former `cfd_core::physics::physics` symbol accordingly; ensure the alias is used consistently in docs and imports.
🤖 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-optim/Cargo.toml`:
- Line 14: Remove the dead feature-gated mesh-export code: delete the entire
generate_mesh_handoff function from crates/cfd-optim/src/lib.rs (including its
blue2mesh import) and remove the MeshExportFailed variant from
crates/cfd-optim/src/error.rs; ensure no other code references
generate_mesh_handoff or MeshExportFailed (remove or adjust those references)
and run cargo build to verify there are no remaining mentions of blue2mesh or
the deleted error variant.
---
Nitpick comments:
In `@crates/cfd-core/src/physics/mod.rs`:
- Line 49: The re-export `pub use constants::{mathematical, physics};` creates
the redundant path `cfd_core::physics::physics`; change the re-export to alias
the inner `physics` module (for example `pub use constants::{mathematical,
physics as physics_constants};`) so callers use
`cfd_core::physics::physics_constants` (or another clear alias you choose) and
update any downstream references to the former `cfd_core::physics::physics`
symbol accordingly; ensure the alias is used consistently in docs and imports.
| [features] | ||
| default = [] | ||
| mesh-export = ["dep:blue2mesh"] | ||
| # mesh-export = ["dep:blue2mesh"] # Disabled per AGENTS.md instructions (external dependency) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check cfd-optim source for any remaining blue2mesh or mesh-export references.
# Test 1: Search for blue2mesh usages. Expect: No results.
echo "=== Test 1: Searching for blue2mesh usages ==="
rg -rn "blue2mesh" --type=rust crates/cfd-optim/
# Test 2: Search for mesh-export feature gates. Expect: No results.
echo "=== Test 2: Searching for mesh-export feature gates ==="
rg -rn 'feature\s*=\s*"mesh-export"' --type=rust crates/cfd-optim/
# Test 3: Also check for cfg(feature = "mesh-export") patterns
echo "=== Test 3: Searching for cfg(feature = \"mesh-export\") ==="
rg -rn 'cfg\s*\(\s*feature\s*=\s*"mesh-export"' --type=rust crates/cfd-optim/Repository: ryancinsight/CFDrs
Length of output: 456
🏁 Script executed:
#!/bin/bash
# Read the actual source files to see the content clearly
echo "=== crates/cfd-optim/src/lib.rs ==="
cat -n crates/cfd-optim/src/lib.rs
echo ""
echo "=== crates/cfd-optim/src/error.rs ==="
cat -n crates/cfd-optim/src/error.rsRepository: ryancinsight/CFDrs
Length of output: 9788
Remove dead code blocks for the disabled mesh-export feature.
The generate_mesh_handoff() function (lines 166–201 in lib.rs) and the MeshExportFailed error variant (line 36 in error.rs) remain gated by #[cfg(feature = "mesh-export")] and will silently become dead code. Delete these blocks entirely rather than leaving them in place:
Affected code locations
crates/cfd-optim/src/lib.rslines 166–201: Remove entiregenerate_mesh_handoff()functioncrates/cfd-optim/src/error.rslines 36–38: RemoveMeshExportFailederror variantcrates/cfd-optim/src/lib.rsline 172: Removeuse blue2mesh::...import (will be deleted with function removal)
🤖 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, Remove the dead feature-gated
mesh-export code: delete the entire generate_mesh_handoff function from
crates/cfd-optim/src/lib.rs (including its blue2mesh import) and remove the
MeshExportFailed variant from crates/cfd-optim/src/error.rs; ensure no other
code references generate_mesh_handoff or MeshExportFailed (remove or adjust
those references) and run cargo build to verify there are no remaining mentions
of blue2mesh or the deleted error variant.
- Workflow: Install `libfontconfig1-dev` on Linux runners to fix `yeslogic-fontconfig-sys` build. - cfd-mesh: Restore `build()` method in `BranchingMeshBuilder`, `SerpentineMeshBuilder`, and `VenturiMeshBuilder` to return `Mesh<T>` (volume mesh) using legacy logic, satisfying `cfd-3d` requirements. Kept `build_surface()` for `IndexedMesh`. - cfd-3d: Disambiguate `sqrt` and `abs` calls in `serpentine/validation.rs` using `Float::sqrt` and `Float::abs`. - cfd-3d: No longer fails on `E0308` (mismatched types) or `E0034` (ambiguity). Co-authored-by: ryancinsight <55164720+ryancinsight@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
crates/cfd-mesh/src/geometry/venturi.rs (1)
223-223: Unused variable_half.
_halfis computed but never referenced. Remove it to avoid dead code.- let _half = T::from_f64(0.5).ok_or_else(|| BuildError("float conv".into()))?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cfd-mesh/src/geometry/venturi.rs` at line 223, Remove the unused temporary variable `_half` created by the call to `T::from_f64(0.5).ok_or_else(|| BuildError("float conv".into()))?` in `venturi.rs`; delete that declaration (the `_half` binding) since it's never referenced and adjust surrounding code if it relied on the conversion side-effect—if the conversion was needed, replace the unused binding with a direct `let _ = ...?;` or use the converted value where intended instead of `_half`.
🤖 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-3d/src/serpentine/validation.rs`:
- Line 63: The Dean number validation currently sets de_valid using an OR with a
positivity check which makes the tolerance check ineffective; change the boolean
expression that defines de_valid (where de_valid is assigned using
Float::abs(de_calc - de_exact_max) < de_tolerance || de_calc > T::zero()) to use
&& so that de_calc must both be within de_tolerance of de_exact_max and positive
(i.e., replace the || with &&), keeping the referenced symbols de_calc,
de_exact_max, de_tolerance, Float::abs and T::zero() intact.
In `@crates/cfd-mesh/src/geometry/branching.rs`:
- Around line 247-272: The add_prism_tets function is creating duplicate Face
entries (e.g., Face::triangle(a0,a1,b1) used as f3 and g0) because
Mesh::add_face blindly appends; this breaks adjacency. Fix by ensuring faces are
deduplicated: either check for an existing face (by canonicalizing vertex order
of Face::triangle(v0,v1,v2) and looking up a face-to-index map) before calling
Mesh::add_face inside add_prism_tets and reuse the existing index, or implement
a mesh finalization routine that scans faces after construction to merge
duplicates and update Cell::tetrahedron indices accordingly; update
add_prism_tets to call the deduplicating add_face helper (or run finalization)
so shared faces produce a single Face index.
In `@crates/cfd-mesh/src/geometry/serpentine.rs`:
- Around line 165-174: build_serpentine_mesh can underflow when b.num_periods ==
0 because n_ax = b.resolution_x.max(4) * b.num_periods may be zero and later
uses n_ax - 1; add a guard at the start of build_serpentine_mesh to validate or
normalize b.num_periods (e.g., require b.num_periods >= 1 or set num_periods =
b.num_periods.max(1)), and if invalid return an Err(BuildError(...)); update any
places that rely on num_periods (n_ax calculation, total_len) to use the
validated/normalized value so subsequent ranges like 0..(n_ax - 1) cannot
underflow.
- Around line 199-216: The loop is creating an invalid Cell::tetrahedron from
six distinct vertices (v00, v01, c0, v10, v11, c1) — tetrahedra must have
exactly 4 vertices — so replace the single add_cell(Cell::tetrahedron(...)) call
with the triangular-prism → 3-tetrahedra decomposition used in branching.rs:
either extract the existing add_prism_tets utility into a shared module (e.g.,
crate::geometry::util) and call add_prism_tets(mesh, [v00, v01, c0, v10, v11,
c1]) here, or inline the same decomposition logic from branching.rs to create
the three proper tetrahedra for the prism defined by v00,v01,c0 at station iz
and v10,v11,c1 at station iz+1; ensure you remove the incorrect
Cell::tetrahedron usage and add the three tetra cells instead.
In `@crates/cfd-mesh/src/geometry/venturi.rs`:
- Around line 260-286: The mesh construction in the loop is creating faces that
collectively reference six distinct vertices (v00, v01, ctr0, v10, v11, ctr1)
but then packs them into a single Cell::tetrahedron, and only one add_cell call
exists despite the comment saying “two tetrahedra per quad column”; replace this
incorrect single-tetra approach by decomposing the prism column into the correct
tetrahedra using the same decomposition as in branching.rs — either call the
shared helper add_prism_tets(mesh, [v00, v01, ctr0, v10, v11, ctr1]) (importing
it) or inline the identical decomposition logic that produces the two (or more)
tet cells from those six vertices, and remove the incorrect
mesh.add_cell(Cell::tetrahedron(...)) usage while keeping the face creation
consistent with the new decomposition.
---
Nitpick comments:
In `@crates/cfd-mesh/src/geometry/venturi.rs`:
- Line 223: Remove the unused temporary variable `_half` created by the call to
`T::from_f64(0.5).ok_or_else(|| BuildError("float conv".into()))?` in
`venturi.rs`; delete that declaration (the `_half` binding) since it's never
referenced and adjust surrounding code if it relied on the conversion
side-effect—if the conversion was needed, replace the unused binding with a
direct `let _ = ...?;` or use the converted value where intended instead of
`_half`.
| let de_calc = solution.dean_number; | ||
| let de_tolerance = de_exact_max * T::from_f64(0.05).unwrap(); // 5% max deviation allowance for local averaging | ||
| let de_valid = (de_calc - de_exact_max).abs() < de_tolerance || de_calc > T::zero(); | ||
| let de_valid = Float::abs(de_calc - de_exact_max) < de_tolerance || de_calc > T::zero(); |
There was a problem hiding this comment.
Dean number validation is effectively a no-op due to || with a positivity check.
The || de_calc > T::zero() branch will be true for any non-trivial flow (Dean number is always positive when there is flow), so the Float::abs(...) tolerance check never gates the validation. If the intent is that the Dean number must be both close to the analytical max and positive, this should use &&.
This is a pre-existing logic issue (the || wasn't introduced by this PR), but since this line was touched, it's worth flagging.
- let de_valid = Float::abs(de_calc - de_exact_max) < de_tolerance || de_calc > T::zero();
+ let de_valid = Float::abs(de_calc - de_exact_max) < de_tolerance && de_calc > T::zero();📝 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.
| let de_valid = Float::abs(de_calc - de_exact_max) < de_tolerance || de_calc > T::zero(); | |
| let de_valid = Float::abs(de_calc - de_exact_max) < de_tolerance && de_calc > T::zero(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/cfd-3d/src/serpentine/validation.rs` at line 63, The Dean number
validation currently sets de_valid using an OR with a positivity check which
makes the tolerance check ineffective; change the boolean expression that
defines de_valid (where de_valid is assigned using Float::abs(de_calc -
de_exact_max) < de_tolerance || de_calc > T::zero()) to use && so that de_calc
must both be within de_tolerance of de_exact_max and positive (i.e., replace the
|| with &&), keeping the referenced symbols de_calc, de_exact_max, de_tolerance,
Float::abs and T::zero() intact.
| /// Decompose a triangular prism defined by two triangles (b0,a0,a1) and (b1,c0,c1) | ||
| /// into 3 tetrahedra and add them to the mesh. | ||
| fn add_prism_tets<T: Copy + RealField>( | ||
| mesh: &mut Mesh<T>, | ||
| b0: usize, a0: usize, a1: usize, | ||
| b1: usize, c0: usize, c1: usize, | ||
| ) { | ||
| // Tet 1: [b0, a0, a1, b1] | ||
| let f0 = mesh.add_face(Face::triangle(b0, a0, a1)); | ||
| let f1 = mesh.add_face(Face::triangle(b0, a0, b1)); | ||
| let f2 = mesh.add_face(Face::triangle(b0, a1, b1)); | ||
| let f3 = mesh.add_face(Face::triangle(a0, a1, b1)); | ||
| mesh.add_cell(Cell::tetrahedron(f0, f1, f2, f3)); | ||
| // Tet 2: [a0, a1, b1, c0] | ||
| let g0 = mesh.add_face(Face::triangle(a0, a1, b1)); | ||
| let g1 = mesh.add_face(Face::triangle(a0, a1, c0)); | ||
| let g2 = mesh.add_face(Face::triangle(a0, b1, c0)); | ||
| let g3 = mesh.add_face(Face::triangle(a1, b1, c0)); | ||
| mesh.add_cell(Cell::tetrahedron(g0, g1, g2, g3)); | ||
| // Tet 3: [a1, b1, c0, c1] | ||
| let h0 = mesh.add_face(Face::triangle(a1, b1, c0)); | ||
| let h1 = mesh.add_face(Face::triangle(a1, b1, c1)); | ||
| let h2 = mesh.add_face(Face::triangle(a1, c0, c1)); | ||
| let h3 = mesh.add_face(Face::triangle(b1, c0, c1)); | ||
| mesh.add_cell(Cell::tetrahedron(h0, h1, h2, h3)); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, find where Mesh<T> is defined
fd -e rs | xargs rg -l "impl.*Mesh" | head -5Repository: ryancinsight/CFDrs
Length of output: 284
🏁 Script executed:
#!/bin/bash
# Look at the structure of the repository
git ls-files crates/cfd-mesh/src | head -20Repository: ryancinsight/CFDrs
Length of output: 786
🏁 Script executed:
#!/bin/bash
# Find Mesh implementation and add_face method
find crates/cfd-mesh/src -name "*.rs" | xargs rg -n "pub fn add_face" -B2 -A10Repository: ryancinsight/CFDrs
Length of output: 2799
🏁 Script executed:
#!/bin/bash
# Check the type signature of Mesh in branching.rs
head -50 crates/cfd-mesh/src/geometry/branching.rs | grep -A5 "use\|fn add_prism"Repository: ryancinsight/CFDrs
Length of output: 506
🏁 Script executed:
#!/bin/bash
# Get full context of the add_prism_tets function
sed -n '247,272p' crates/cfd-mesh/src/geometry/branching.rsRepository: ryancinsight/CFDrs
Length of output: 1264
🏁 Script executed:
#!/bin/bash
# Look for the FaceStore implementation and add_triangle method
rg -n "impl.*FaceStore\|fn add_triangle" crates/cfd-mesh/src -A10Repository: ryancinsight/CFDrs
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Check for any rebuild, dedup, or finalization methods in Mesh
rg -n "rebuild|dedup|finalize|merge_faces" crates/cfd-mesh/src/mesh.rs -B2 -A5Repository: ryancinsight/CFDrs
Length of output: 2520
🏁 Script executed:
#!/bin/bash
# Get full Mesh<T> implementation
sed -n '100,200p' crates/cfd-mesh/src/mesh.rsRepository: ryancinsight/CFDrs
Length of output: 2852
🏁 Script executed:
#!/bin/bash
# Check Face implementation and equality
rg -n "struct Face\|impl.*Face\|#\[derive" crates/cfd-mesh/src/topology -A3 | head -50Repository: ryancinsight/CFDrs
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Search for where add_prism_tets is called
rg -n "add_prism_tets" crates/cfd-mesh/src -B5 -A5Repository: ryancinsight/CFDrs
Length of output: 2708
🏁 Script executed:
#!/bin/bash
# Check the return type and how the mesh is used after add_prism_tets
sed -n '200,250p' crates/cfd-mesh/src/geometry/branching.rsRepository: ryancinsight/CFDrs
Length of output: 1803
🏁 Script executed:
#!/bin/bash
# Look for the function signature that returns the mesh
rg -n "pub fn.*branching\|-> Result<.*Mesh" crates/cfd-mesh/src/geometry/branching.rs -A2Repository: ryancinsight/CFDrs
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Check the Face struct definition
rg -n "pub struct Face\|pub enum Face" crates/cfd-mesh/src/topology -A10Repository: ryancinsight/CFDrs
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Find the topology module file
fd -e rs topology crates/cfd-mesh/srcRepository: ryancinsight/CFDrs
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Search for Face in topology files
rg -n "struct Face" crates/cfd-mesh/src -A5Repository: ryancinsight/CFDrs
Length of output: 2500
🏁 Script executed:
#!/bin/bash
# Check the full function signature with return type
sed -n '100,250p' crates/cfd-mesh/src/geometry/branching.rs | head -100Repository: ryancinsight/CFDrs
Length of output: 3749
🏁 Script executed:
#!/bin/bash
# Check IndexedMesh::add_face_with_region method
rg -n "pub fn add_face_with_region" crates/cfd-mesh/src/mesh.rs -A10Repository: ryancinsight/CFDrs
Length of output: 365
🏁 Script executed:
#!/bin/bash
# Look at FaceStore::add_triangle method for deduplication logic
rg -n "fn add_triangle" crates/cfd-mesh/src/storage/face_store.rs -A15Repository: ryancinsight/CFDrs
Length of output: 693
🏁 Script executed:
#!/bin/bash
# Check if FaceStore or VertexPool has any deduplication for faces
rg -n "contains\|dedup\|find\|insert" crates/cfd-mesh/src/storage/face_store.rs -B2 -A3Repository: ryancinsight/CFDrs
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Check the push method in FaceStore
rg -n "fn push" crates/cfd-mesh/src/storage/face_store.rs -A5Repository: ryancinsight/CFDrs
Length of output: 244
🏁 Script executed:
#!/bin/bash
# Check EdgeStore::from_face_store to understand what rebuild_edges does
rg -n "from_face_store\|impl.*EdgeStore" crates/cfd-mesh/src/storage -A30 | head -80Repository: ryancinsight/CFDrs
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Check if the branching function or its caller invokes rebuild_edges
rg -n "rebuild_edges\|\.build()" crates/cfd-mesh/src/geometry/branching.rsRepository: ryancinsight/CFDrs
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Get the full return statement of the branching function
sed -n '240,250p' crates/cfd-mesh/src/geometry/branching.rsRepository: ryancinsight/CFDrs
Length of output: 401
🏁 Script executed:
#!/bin/bash
# Look for all versions of add_prism_tets and check context around line 306
sed -n '300,315p' crates/cfd-mesh/src/geometry/branching.rsRepository: ryancinsight/CFDrs
Length of output: 676
🏁 Script executed:
#!/bin/bash
# Get more context around the call to add_prism_tets at line 306
sed -n '295,310p' crates/cfd-mesh/src/geometry/branching.rsRepository: ryancinsight/CFDrs
Length of output: 594
🏁 Script executed:
#!/bin/bash
# Check the exact type of mesh variable where add_prism_tets is called
sed -n '80,130p' crates/cfd-mesh/src/geometry/branching.rsRepository: ryancinsight/CFDrs
Length of output: 2077
🏁 Script executed:
#!/bin/bash
# Check if there are multiple versions of add_prism_tets
rg -n "fn add_prism_tets" crates/cfd-mesh/src -A20Repository: ryancinsight/CFDrs
Length of output: 1986
🏁 Script executed:
#!/bin/bash
# Look for the full function containing the call to add_prism_tets to understand the mesh type
sed -n '65,250p' crates/cfd-mesh/src/geometry/branching.rs | head -150Repository: ryancinsight/CFDrs
Length of output: 5414
🏁 Script executed:
#!/bin/bash
# Check EdgeStore implementation to see what happens when edges are rebuilt
rg -n "impl.*EdgeStore\|from_face_store" crates/cfd-mesh/src/storage -A50 | head -100Repository: ryancinsight/CFDrs
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Find build_branching_mesh function to see how the Mesh<T> is used
rg -n "fn build_branching_mesh" crates/cfd-mesh/src/geometry/branching.rs -A50 | head -100Repository: ryancinsight/CFDrs
Length of output: 2267
🏁 Script executed:
#!/bin/bash
# Check if Mesh<T> is ever converted to IndexedMesh or if rebuild_edges is called
rg -n "rebuild_edges\|IndexedMesh\|into\|from" crates/cfd-mesh/src/geometry/branching.rsRepository: ryancinsight/CFDrs
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Check what Mesh<T> is used for - look at its return/export
sed -n '1,70p' crates/cfd-mesh/src/mesh.rsRepository: ryancinsight/CFDrs
Length of output: 2636
Duplicate internal faces break mesh adjacency for FEM/FVM solvers.
The Mesh<T>::add_face() method simply appends faces without deduplication. Shared triangular faces between adjacent tets (e.g., (a0, a1, b1) appears as both f3 in Tet 1 and g0 in Tet 2) are stored as separate Face objects with different indices. Since the mesh is returned directly without a finalization or face-deduplication pass, downstream FEM/FVM solvers cannot discover inter-cell adjacency via face topology.
This requires either:
- Deduplicating faces during construction by checking if
Face::triangle(v0, v1, v2)already exists before callingadd_face, or - Adding a finalization step to merge duplicate faces before returning the mesh.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/cfd-mesh/src/geometry/branching.rs` around lines 247 - 272, The
add_prism_tets function is creating duplicate Face entries (e.g.,
Face::triangle(a0,a1,b1) used as f3 and g0) because Mesh::add_face blindly
appends; this breaks adjacency. Fix by ensuring faces are deduplicated: either
check for an existing face (by canonicalizing vertex order of
Face::triangle(v0,v1,v2) and looking up a face-to-index map) before calling
Mesh::add_face inside add_prism_tets and reuse the existing index, or implement
a mesh finalization routine that scans faces after construction to merge
duplicates and update Cell::tetrahedron indices accordingly; update
add_prism_tets to call the deduplicating add_face helper (or run finalization)
so shared faces produce a single Face index.
| fn build_serpentine_mesh<T: Copy + RealField + Float + FromPrimitive>( | ||
| b: SerpentineMeshBuilder<T>, | ||
| ) -> Result<Mesh<T>, BuildError> { | ||
| let n_ax = b.resolution_x.max(4) * b.num_periods; | ||
| let n_ang = (b.resolution_y.max(2) * 4).max(4); | ||
| let two_pi = T::from_f64(std::f64::consts::TAU) | ||
| .ok_or_else(|| BuildError("float conv".into()))?; | ||
| let r = b.diameter / T::from_f64(2.0).ok_or_else(|| BuildError("float conv".into()))?; | ||
| let total_len = b.wavelength * T::from_usize(b.num_periods) | ||
| .ok_or_else(|| BuildError("float conv".into()))?; |
There was a problem hiding this comment.
Potential usize underflow if num_periods == 0.
If a caller sets num_periods to 0 via with_periods(0), then n_ax = resolution_x.max(4) * 0 = 0. Line 200's 0..(n_ax - 1) becomes 0..usize::MAX in release (or panics in debug). Consider adding a guard at the top of build_serpentine_mesh:
fn build_serpentine_mesh<T: Copy + RealField + Float + FromPrimitive>(
b: SerpentineMeshBuilder<T>,
) -> Result<Mesh<T>, BuildError> {
+ if b.num_periods == 0 {
+ return Err(BuildError("num_periods must be > 0".into()));
+ }
let n_ax = b.resolution_x.max(4) * b.num_periods;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/cfd-mesh/src/geometry/serpentine.rs` around lines 165 - 174,
build_serpentine_mesh can underflow when b.num_periods == 0 because n_ax =
b.resolution_x.max(4) * b.num_periods may be zero and later uses n_ax - 1; add a
guard at the start of build_serpentine_mesh to validate or normalize
b.num_periods (e.g., require b.num_periods >= 1 or set num_periods =
b.num_periods.max(1)), and if invalid return an Err(BuildError(...)); update any
places that rely on num_periods (n_ax calculation, total_len) to use the
validated/normalized value so subsequent ranges like 0..(n_ax - 1) cannot
underflow.
| // Cells | ||
| for iz in 0..(n_ax - 1) { | ||
| let base0 = iz * verts_per_ring; | ||
| let base1 = (iz + 1) * verts_per_ring; | ||
| for ia in 0..n_ang { | ||
| let ia1 = (ia + 1) % n_ang; | ||
| let v00 = base0 + 1 + ia; | ||
| let v01 = base0 + 1 + ia1; | ||
| let v10 = base1 + 1 + ia; | ||
| let v11 = base1 + 1 + ia1; | ||
| let c0 = base0; | ||
| let c1 = base1; | ||
| let f0 = mesh.add_face(Face::triangle(v00, v01, c0)); | ||
| let f1 = mesh.add_face(Face::triangle(v10, v11, c1)); | ||
| let f2 = mesh.add_face(Face::triangle(v00, v10, v01)); | ||
| let f3 = mesh.add_face(Face::triangle(v01, v10, v11)); | ||
| mesh.add_cell(Cell::tetrahedron(f0, f1, f2, f3)); | ||
| } |
There was a problem hiding this comment.
Invalid tetrahedra: 6 vertices packed into a single Cell::tetrahedron that requires exactly 4.
Each "cell" here references faces spanning 6 distinct vertices (v00, v01, c0 at axial station iz, and v10, v11, c1 at station iz+1). A tetrahedron is defined by exactly 4 vertices. Additionally, the 4 faces don't form a closed polyhedron — multiple bounding faces are missing.
The branching builder (branching.rs) handles the identical geometry correctly by decomposing each 6-vertex triangular prism into 3 proper tetrahedra via add_prism_tets. The same approach should be used here.
Proposed fix: use the same prism decomposition as branching.rs
Either extract add_prism_tets into a shared utility and call it here, or inline the decomposition:
// Cells
for iz in 0..(n_ax - 1) {
let base0 = iz * verts_per_ring;
let base1 = (iz + 1) * verts_per_ring;
for ia in 0..n_ang {
let ia1 = (ia + 1) % n_ang;
- let v00 = base0 + 1 + ia;
- let v01 = base0 + 1 + ia1;
- let v10 = base1 + 1 + ia;
- let v11 = base1 + 1 + ia1;
- let c0 = base0;
- let c1 = base1;
- let f0 = mesh.add_face(Face::triangle(v00, v01, c0));
- let f1 = mesh.add_face(Face::triangle(v10, v11, c1));
- let f2 = mesh.add_face(Face::triangle(v00, v10, v01));
- let f3 = mesh.add_face(Face::triangle(v01, v10, v11));
- mesh.add_cell(Cell::tetrahedron(f0, f1, f2, f3));
+ add_prism_tets(
+ &mut mesh,
+ base0, base0 + 1 + ia, base0 + 1 + ia1,
+ base1, base1 + 1 + ia, base1 + 1 + ia1,
+ );
}add_prism_tets is currently defined in branching.rs — consider moving it to a shared module (e.g., crate::geometry::util) so all builders can reuse it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/cfd-mesh/src/geometry/serpentine.rs` around lines 199 - 216, The loop
is creating an invalid Cell::tetrahedron from six distinct vertices (v00, v01,
c0, v10, v11, c1) — tetrahedra must have exactly 4 vertices — so replace the
single add_cell(Cell::tetrahedron(...)) call with the triangular-prism →
3-tetrahedra decomposition used in branching.rs: either extract the existing
add_prism_tets utility into a shared module (e.g., crate::geometry::util) and
call add_prism_tets(mesh, [v00, v01, c0, v10, v11, c1]) here, or inline the same
decomposition logic from branching.rs to create the three proper tetrahedra for
the prism defined by v00,v01,c0 at station iz and v10,v11,c1 at station iz+1;
ensure you remove the incorrect Cell::tetrahedron usage and add the three tetra
cells instead.
| // Create faces and cells (wedge elements between axial slices). | ||
| for iz in 0..(n_ax - 1) { | ||
| let base0 = iz * verts_per_ring; | ||
| let base1 = (iz + 1) * verts_per_ring; | ||
|
|
||
| let _z0 = z_vals[iz]; | ||
| let _z1 = z_vals[iz + 1]; | ||
|
|
||
| for ia in 0..n_ang { | ||
| let ia1 = (ia + 1) % n_ang; | ||
|
|
||
| // Four vertices of the wedge quad between the two rings. | ||
| let v00 = base0 + 1 + ia; | ||
| let v01 = base0 + 1 + ia1; | ||
| let v10 = base1 + 1 + ia; | ||
| let v11 = base1 + 1 + ia1; | ||
| let ctr0 = base0; | ||
| let ctr1 = base1; | ||
|
|
||
| // Create two tetrahedra per quad column. | ||
| let f0 = mesh.add_face(Face::triangle(v00, v01, ctr0)); | ||
| let f1 = mesh.add_face(Face::triangle(v10, v11, ctr1)); | ||
| let f2 = mesh.add_face(Face::triangle(v00, v10, v01)); | ||
| let f3 = mesh.add_face(Face::triangle(v01, v10, v11)); | ||
|
|
||
| mesh.add_cell(Cell::tetrahedron(f0, f1, f2, f3)); | ||
| } |
There was a problem hiding this comment.
Same invalid tetrahedra issue as in serpentine.rs — 6 vertices in a single Cell::tetrahedron.
The faces reference 6 distinct vertices (v00, v01, ctr0, v10, v11, ctr1) but are packed into one tetrahedron cell. Additionally, the comment on Line 279 says "Create two tetrahedra per quad column" but only one add_cell is made.
This is the same issue flagged in serpentine.rs. The fix from branching.rs (add_prism_tets) should be applied here as well.
Proposed fix
for ia in 0..n_ang {
let ia1 = (ia + 1) % n_ang;
- // Four vertices of the wedge quad between the two rings.
- let v00 = base0 + 1 + ia;
- let v01 = base0 + 1 + ia1;
- let v10 = base1 + 1 + ia;
- let v11 = base1 + 1 + ia1;
- let ctr0 = base0;
- let ctr1 = base1;
-
- // Create two tetrahedra per quad column.
- let f0 = mesh.add_face(Face::triangle(v00, v01, ctr0));
- let f1 = mesh.add_face(Face::triangle(v10, v11, ctr1));
- let f2 = mesh.add_face(Face::triangle(v00, v10, v01));
- let f3 = mesh.add_face(Face::triangle(v01, v10, v11));
-
- mesh.add_cell(Cell::tetrahedron(f0, f1, f2, f3));
+ add_prism_tets(
+ &mut mesh,
+ base0, base0 + 1 + ia, base0 + 1 + ia1,
+ base1, base1 + 1 + ia, base1 + 1 + ia1,
+ );
}This requires either importing add_prism_tets from a shared location or inlining the same decomposition.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/cfd-mesh/src/geometry/venturi.rs` around lines 260 - 286, The mesh
construction in the loop is creating faces that collectively reference six
distinct vertices (v00, v01, ctr0, v10, v11, ctr1) but then packs them into a
single Cell::tetrahedron, and only one add_cell call exists despite the comment
saying “two tetrahedra per quad column”; replace this incorrect single-tetra
approach by decomposing the prism column into the correct tetrahedra using the
same decomposition as in branching.rs — either call the shared helper
add_prism_tets(mesh, [v00, v01, ctr0, v10, v11, ctr1]) (importing it) or inline
the identical decomposition logic that produces the two (or more) tet cells from
those six vertices, and remove the incorrect
mesh.add_cell(Cell::tetrahedron(...)) usage while keeping the face creation
consistent with the new decomposition.
1 participant
Consolidate
cfd-core/src/physics/mod.rsby removing theapisubmodule and directly re-exporting domain types. This simplifies the module hierarchy and improves API clarity. Also added missingcavitationexports. Addressedcrates/cfd-optim/Cargo.tomldependency commenting as per instructions.PR created automatically by Jules for task 10647267707929497211 started by @ryancinsight
High-level PR Summary
This PR refactors the
cfd-core::physicsmodule by eliminating the nestedapisubmodule and moving all re-exports directly intomod.rs. This flattens the module hierarchy so consumers can import types directly fromcfd_core::physics::*rather than throughphysics::api::*. The PR also adds missingcavitationexports (likeCavitationModel,RayleighPlesset, etc.) and comments out theblue2meshdependency incfd-optim/Cargo.tomlas per project instructions. TheCargo.lockchanges reflect removal of unused transitive dependencies fromblue2mesh.⏱️ Estimated Review Time: 30-90 minutes
💡 Review Order Suggestion
crates/cfd-core/src/physics/mod.rscrates/cfd-optim/Cargo.tomlCargo.lockSummary by CodeRabbit