refactor(cfd-core): flatten physics module structure#259
refactor(cfd-core): flatten physics module structure#259ryancinsight wants to merge 2 commits intomainfrom
Conversation
…list - Remove `api` submodule from `crates/cfd-core/src/physics/mod.rs`. - Re-export `boundary`, `cavitation`, `constants`, `fluid`, `fluid_dynamics`, `hemolysis`, `material`, and `values` types directly from `physics/mod.rs`. - Update `CHECKLIST.md` to mark physics re-exports, external references update, and duplicate elimination tasks as complete. - Temporarily comment out missing `blue2mesh` dependency in `crates/cfd-optim/Cargo.toml` to allow compilation. This change simplifies the `cfd-core` physics API surface and aligns with the deep vertical tree architecture. 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. |
📝 WalkthroughWalkthroughUpdates checklist task statuses and formatting; removes the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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, 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 refactors 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 effectively flattens the physics module structure in cfd-core, which simplifies the public API and improves usability. The changes align well with the stated goal of creating a "Deep Vertical Tree" architecture. The temporary disabling of the blue2mesh dependency to fix CI is a pragmatic step. My review includes a few suggestions for improving maintainability by adding TODO comments for these temporary changes and cleaning up obsolete TODOs in the CHECKLIST.md file.
| - [x] **Phase 2: Physics Consolidation (REST-001)** | ||
| - [x] Verify `fluid`, `material`, `boundary`, `constants` are in `cfd-core/src/physics/` ✅ | ||
| - [ ] Update re-exports and documentation | ||
| - [x] Update re-exports and documentation ✅ |
There was a problem hiding this comment.
Now that this task is complete, the corresponding TODO on line 56 is obsolete and can be removed to keep the checklist clean.
| - [x] Rename `domain` to `geometry` in `cfd-core` ✅ | ||
| - [x] Move `rhie_chow.rs` to `physics/fluid_dynamics` and remove `interpolation` ✅ | ||
| - [ ] Update all external references to `cfd_core::domain` to `cfd_core::geometry` | ||
| - [x] Update all external references to `cfd_core::domain` to `cfd_core::geometry` ✅ |
There was a problem hiding this comment.
With this task completed, the TODO on line 62 is now resolved and can be removed.
| - [ ] **Phase 6: Duplicate Elimination (REST-005)** | ||
| - [x] Eliminate SIMD duplication in `cfd-math` ✅ | ||
| - [ ] Eliminate WENO duplication in `cfd-math` (src/high_order/weno.rs vs src/spatial/weno.rs) | ||
| - [x] Eliminate WENO duplication in `cfd-math` (src/high_order/weno.rs vs src/spatial/weno.rs) ✅ |
There was a problem hiding this comment.
Since this task completes the Duplicate Elimination phase, the summary TODO on line 72 is now obsolete and can be removed.
| [features] | ||
| default = [] | ||
| mesh-export = ["dep:blue2mesh"] | ||
| # mesh-export = ["dep:blue2mesh"] |
There was a problem hiding this comment.
Since this feature is temporarily disabled for CI purposes, it's a good practice to add a TODO comment. This will act as a reminder to re-enable it once the underlying dependency issues are resolved.
| # mesh-export = ["dep:blue2mesh"] | |
| # TODO: [CI-FIX] Re-enable mesh-export feature once blue2mesh dependency issues are resolved. | |
| # mesh-export = ["dep:blue2mesh"] |
| cfd-schematics.workspace = true | ||
|
|
||
| blue2mesh = { path = "../../../blue2mesh", optional = true, default-features = false, features = ["mesh-generation", "cfd-export", "stl-export"] } | ||
| # blue2mesh = { path = "../../../blue2mesh", optional = true, default-features = false, features = ["mesh-generation", "cfd-export", "stl-export"] } |
There was a problem hiding this comment.
To ensure this temporary change isn't forgotten, consider adding a TODO comment explaining why this dependency is commented out and when it should be restored.
| # blue2mesh = { path = "../../../blue2mesh", optional = true, default-features = false, features = ["mesh-generation", "cfd-export", "stl-export"] } | |
| # TODO: [CI-FIX] Uncomment blue2mesh dependency once CI issues are resolved. It is a local path dependency that may not be available in the CI environment. | |
| # blue2mesh = { path = "../../../blue2mesh", optional = true, default-features = false, features = ["mesh-generation", "cfd-export", "stl-export"] } |
![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 b41108f..a80a527
✨ No bugs found, your code is sparkling clean
✅ Files analyzed, no issues (4)
• CHECKLIST.md
• Cargo.lock
• crates/cfd-core/src/physics/mod.rs
• crates/cfd-optim/Cargo.toml
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/cfd-optim/Cargo.toml (1)
14-27:⚠️ Potential issue | 🟠 MajorPreserve feature compatibility while dependency is temporarily unavailable.
Commenting out the feature entirely is a semver-facing break for consumers that still enable
mesh-export. Keep a temporary compatibility feature and gate the export example explicitly.Proposed manifest adjustment
[features] default = [] -# mesh-export = ["dep:blue2mesh"] +mesh-export = [] # temporary compatibility stub until blue2mesh is restored [dependencies] -# blue2mesh = { path = "../../../blue2mesh", optional = true, default-features = false, features = ["mesh-generation", "cfd-export", "stl-export"] } +# TODO: restore when local dependency is available again. +# blue2mesh = { path = "../../../blue2mesh", optional = true, default-features = false, features = ["mesh-generation", "cfd-export", "stl-export"] } [[example]] name = "sdt_export" path = "examples/sdt_export.rs" +required-features = ["mesh-export"]Run this to verify current feature/dependency usage before finalizing:
#!/bin/bash set -euo pipefail # 1) Find Cargo declarations of mesh-export / blue2mesh rg -nP --type=toml '\bmesh-export\b|\bblue2mesh\b' # 2) Find code gated by mesh-export or direct blue2mesh usage rg -nP --type=rust 'cfg\s*\(\s*feature\s*=\s*"mesh-export"\s*\)|\bblue2mesh\b' # 3) Inspect example feature gating in this crate manifest rg -nP '^\[\[example\]\]|^name\s*=|^required-features\s*=' crates/cfd-optim/Cargo.toml🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cfd-optim/Cargo.toml` around lines 14 - 27, The crate currently removed the blue2mesh dependency which breaks the public feature "mesh-export"; restore compatibility by adding an explicit feature entry in crates/cfd-optim/Cargo.toml (e.g. under [features] add mesh-export = [] or mesh-export = ["dep:blue2mesh"] as a temporary no-op) and keep the optional dependency declaration for blue2mesh commented/unavailable, then gate the example or export code by the "mesh-export" feature (inspect any example blocks with required-features or cfg(feature = "mesh-export") and ensure they reference mesh-export) so consumers enabling mesh-export still compile until blue2mesh is reintroduced.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHECKLIST.md`:
- Around line 52-60: The checklist has conflicting status markers: tasks under
"Phase 2: Physics Consolidation (REST-001)" and "Phase 3: Geometry Consolidation
(REST-002)" are marked complete but the nearby "TODO(MEDIUM): Update physics
module re-exports and documentation after consolidation." line still indicates
pending work; update CHECKLIST.md to make it a single source of truth by either
removing or marking that TODO as done and ensuring the Phase 2/Phase 3 entries
(the checked boxes for fluid, material, boundary, constants, rename
domain→geometry, move rhie_chow.rs, update cfd_core::domain→cfd_core::geometry)
consistently reflect completion, and update any adjacent explanatory text so the
document no longer claims those updates are pending.
---
Outside diff comments:
In `@crates/cfd-optim/Cargo.toml`:
- Around line 14-27: The crate currently removed the blue2mesh dependency which
breaks the public feature "mesh-export"; restore compatibility by adding an
explicit feature entry in crates/cfd-optim/Cargo.toml (e.g. under [features] add
mesh-export = [] or mesh-export = ["dep:blue2mesh"] as a temporary no-op) and
keep the optional dependency declaration for blue2mesh commented/unavailable,
then gate the example or export code by the "mesh-export" feature (inspect any
example blocks with required-features or cfg(feature = "mesh-export") and ensure
they reference mesh-export) so consumers enabling mesh-export still compile
until blue2mesh is reintroduced.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
CHECKLIST.mdcrates/cfd-core/src/physics/mod.rscrates/cfd-optim/Cargo.toml
| - [x] **Phase 2: Physics Consolidation (REST-001)** | ||
| - [x] Verify `fluid`, `material`, `boundary`, `constants` are in `cfd-core/src/physics/` ✅ | ||
| - [ ] Update re-exports and documentation | ||
| - [x] Update re-exports and documentation ✅ | ||
|
|
||
| **TODO(MEDIUM): Update physics module re-exports and documentation after consolidation. Dependencies: REST-001 completion. Reference: SOLID principles for clean module boundaries.** | ||
| - [ ] **Phase 3: Geometry Consolidation (REST-002)** | ||
| - [x] Rename `domain` to `geometry` in `cfd-core` ✅ | ||
| - [x] Move `rhie_chow.rs` to `physics/fluid_dynamics` and remove `interpolation` ✅ | ||
| - [ ] Update all external references to `cfd_core::domain` to `cfd_core::geometry` | ||
| - [x] Update all external references to `cfd_core::domain` to `cfd_core::geometry` ✅ |
There was a problem hiding this comment.
Status lines now conflict with nearby TODO statements.
Lines 52–55 and Line 60 mark these tasks complete, but the nearby TODO notes still say those updates are pending. Please reconcile to keep the checklist as a single source of truth.
Suggested cleanup
-**TODO(MEDIUM): Update physics module re-exports and documentation after consolidation. Dependencies: REST-001 completion. Reference: SOLID principles for clean module boundaries.**
+**TODO(MEDIUM): (Resolved) Physics module re-exports and documentation updated.**
-**TODO(MEDIUM): Update all external references from cfd_core::domain to cfd_core::geometry post-renaming. Dependencies: REST-002 completion. Mathematical foundation: Maintain API consistency for bounded contexts.**
+**TODO(MEDIUM): (Resolved) External references updated to cfd_core::geometry.**🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CHECKLIST.md` around lines 52 - 60, The checklist has conflicting status
markers: tasks under "Phase 2: Physics Consolidation (REST-001)" and "Phase 3:
Geometry Consolidation (REST-002)" are marked complete but the nearby
"TODO(MEDIUM): Update physics module re-exports and documentation after
consolidation." line still indicates pending work; update CHECKLIST.md to make
it a single source of truth by either removing or marking that TODO as done and
ensuring the Phase 2/Phase 3 entries (the checked boxes for fluid, material,
boundary, constants, rename domain→geometry, move rhie_chow.rs, update
cfd_core::domain→cfd_core::geometry) consistently reflect completion, and update
any adjacent explanatory text so the document no longer claims those updates are
pending.
The `Performance Benchmarking` workflow was failing on Linux due to missing `fontconfig` development libraries required by `yeslogic-fontconfig-sys` (a dependency of `plotters`). This commit adds `libfontconfig1-dev` to the `apt-get install` command in the Linux dependency installation step. Fixes: CI failure in `Performance Benchmarking` job on ubuntu-latest. Co-authored-by: ryancinsight <55164720+ryancinsight@users.noreply.github.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/performance-benchmarking.yml (1)
166-168: Downstream steps reference commented-outregression_checkstep.The
regression_checkstep (lines 107-122) is commented out, but this step and the one at line 203 still referencesteps.regression_check.outputs.critical_regression. These conditions will never evaluate totruesince the output doesn't exist.Similarly, line 248 references
needs.benchmark.outputs.critical_regression, but thebenchmarkjob never sets this output.This is pre-existing and not part of this PR, but worth noting for a future cleanup.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/performance-benchmarking.yml around lines 166 - 168, Downstream steps reference a missing step/output: update references to steps.regression_check.outputs.critical_regression and needs.benchmark.outputs.critical_regression so they no longer depend on a non-existent output—either re-enable the regression_check step to emit outputs or remove/replace the conditional checks in the steps that use steps.regression_check.outputs.critical_regression and needs.benchmark.outputs.critical_regression (e.g., remove the if: clauses or replace with a safe flag that the benchmark job actually sets); ensure the regression_check step (named regression_check) or the benchmark job sets outputs.authoritative names if you choose to re-enable them.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/performance-benchmarking.yml:
- Around line 166-168: Downstream steps reference a missing step/output: update
references to steps.regression_check.outputs.critical_regression and
needs.benchmark.outputs.critical_regression so they no longer depend on a
non-existent output—either re-enable the regression_check step to emit outputs
or remove/replace the conditional checks in the steps that use
steps.regression_check.outputs.critical_regression and
needs.benchmark.outputs.critical_regression (e.g., remove the if: clauses or
replace with a safe flag that the benchmark job actually sets); ensure the
regression_check step (named regression_check) or the benchmark job sets
outputs.authoritative names if you choose to re-enable them.
1 participant
This PR flattens the
cfd-corephysics module structure by removing the intermediateapisubmodule and re-exporting all physics-related types directly fromcrates/cfd-core/src/physics/mod.rs. This aligns with the "Deep Vertical Tree" architecture and simplifies imports for users. It also updates theCHECKLIST.mdto reflect the completion of physics re-exports, external references updates, and duplicate elimination incfd-math. Additionally, it temporarily comments out a missing local dependencyblue2meshincrates/cfd-optimto fix build issues in the CI environment.PR created automatically by Jules for task 7256050132964309749 started by @ryancinsight
High-level PR Summary
This PR flattens the
cfd-corephysics module by removing the nestedapisubmodule and re-exporting all physics types directly fromphysics/mod.rs, making imports more straightforward for users. It also updates the project checklist to mark several completed tasks (physics re-exports, geometry references, and WENO duplication elimination), and temporarily comments out a missingblue2meshdependency incfd-optimto fix CI build issues. The changes align with a "Deep Vertical Tree" architecture and eliminate an unnecessary layer of indirection in the module structure.⏱️ Estimated Review Time: 15-30 minutes
💡 Review Order Suggestion
crates/cfd-core/src/physics/mod.rscrates/cfd-optim/Cargo.tomlCHECKLIST.mdCargo.lockSummary by CodeRabbit