Skip to content

Fix validation logic to only check external boundary nodes#249

Open
ryancinsight wants to merge 1 commit intomainfrom
fix-fem-validation-boundary-nodes-61686539264122540
Open

Fix validation logic to only check external boundary nodes#249
ryancinsight wants to merge 1 commit intomainfrom
fix-fem-validation-boundary-nodes-61686539264122540

Conversation

@ryancinsight
Copy link
Owner

@ryancinsight ryancinsight commented Feb 16, 2026

Updated StokesFlowProblem validation to only enforce boundary conditions on topologically external nodes (cell_count == 1). Internal marked faces are now ignored during validation, preventing false positives where internal nodes were flagged as missing BCs. Implemented get_external_boundary_nodes() helper method and updated solver logic. Also disabled the broken scheme dependency in crates/cfd-1d.


PR created automatically by Jules for task 61686539264122540 started by @ryancinsight

High-level PR Summary

This PR fixes a validation bug in the Stokes flow problem where internal nodes on marked faces were incorrectly flagged as missing boundary conditions. The fix introduces a get_external_boundary_nodes() method that filters out topologically internal faces (referenced by multiple cells) and only returns nodes on truly external boundaries. The validation and solver logic now use this method instead of the broader get_boundary_nodes() implementation. Additionally, the broken scheme dependency integration in crates/cfd-1d has been disabled by commenting out the feature and dependency references.

⏱️ Estimated Review Time: 30-90 minutes

💡 Review Order Suggestion
Order File Path
1 crates/cfd-3d/src/fem/problem.rs
2 crates/cfd-3d/src/fem/solver.rs
3 crates/cfd-1d/Cargo.toml
4 Cargo.toml
5 Cargo.lock

Need help? Join our Discord

Summary by CodeRabbit

  • Bug Fixes

    • Corrected boundary condition handling to apply only to external boundary nodes, preventing unintended application to internal mesh faces.
    • Re-enabled validation checks in the solver for earlier error detection.
  • Chores

    • Disabled scheme-integration feature flag and related dependencies.

Updated StokesFlowProblem validation to only enforce boundary conditions on topologically external nodes (cell_count == 1). Internal marked faces are now ignored during validation, preventing false positives where internal nodes were flagged as missing BCs. Implemented get_external_boundary_nodes() helper method and updated solver logic. Also disabled the broken `scheme` dependency in `crates/cfd-1d`.

Co-authored-by: ryancinsight <55164720+ryancinsight@users.noreply.github.com>
@google-labs-jules
Copy link
Contributor

👋 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 @jules. You can find this option in the Pull Request section of your global Jules UI settings. You can always switch back!

New to Jules? Learn more at jules.google/docs.


For security, I will only act on instructions from the user who triggered this task.

@coderabbitai
Copy link

coderabbitai bot commented Feb 16, 2026

📝 Walkthrough

Walkthrough

This PR disables scheme-integration features across Cargo configurations and introduces a get_external_boundary_nodes() method to the Stokes flow problem validator, distinguishing external from internal boundary nodes.

Changes

Cohort / File(s) Summary
Feature Flag Disabling
Cargo.toml, crates/cfd-1d/Cargo.toml
Commented out scheme-integration feature flags and corresponding scheme dependencies, effectively disabling them without deletion.
Stokes Flow Boundary Validation
crates/cfd-3d/src/fem/problem.rs
Added public method get_external_boundary_nodes() to collect unique vertex indices on external topological boundary faces in sorted order. Updated validate() to use this new method instead of get_boundary_nodes(). Added test helpers and tests validating behavior with internal vs. external faces.
Boundary Condition Application
crates/cfd-3d/src/fem/solver.rs
Re-enabled problem validation in solve() and switched boundary extraction from get_boundary_nodes() to get_external_boundary_nodes() in apply_boundary_conditions_block().

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

🐰 The boundaries now see outer walls so clear,
Internal faces fade from validator's ear,
Scheme steps back as features rest,
External nodes rise—the boundary's best! ✨

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly and accurately describes the main change: fixing validation logic to only check external boundary nodes, which is the primary focus of the PR.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Merge Conflict Detection ✅ Passed ✅ No merge conflicts detected when merging into main

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix-fem-validation-boundary-nodes-61686539264122540

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@gemini-code-assist
Copy link
Contributor

Summary of Changes

Hello @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 improves the robustness of the StokesFlowProblem by correcting its boundary condition validation logic. It ensures that only nodes on the actual external boundaries of the mesh are considered for boundary condition checks, eliminating erroneous warnings for internal nodes. The solver now correctly utilizes this refined validation, and the project's dependency tree has been streamlined by removing an unused scheme dependency and its associated transitive packages.

Highlights

  • Validation Logic Refinement: The StokesFlowProblem validation now exclusively checks for boundary conditions on 'topologically external nodes', effectively ignoring internal faces that might be marked as boundaries. This change resolves false positives where internal nodes were incorrectly flagged for missing boundary conditions.
  • New Helper Method for External Boundary Nodes: A new public method, get_external_boundary_nodes(), has been introduced in StokesFlowProblem to accurately identify and retrieve only the nodes located on the true external boundary of the mesh, excluding internal interfaces.
  • Solver Logic Update: The StokesFlowSolver has been updated to re-enable the problem.validate() call, leveraging the newly refined validation logic. Additionally, the assemble_system method now uses get_external_boundary_nodes() to correctly identify unconstrained boundary nodes.
  • Dependency Cleanup: The scheme dependency has been disabled in crates/cfd-1d and removed from the root Cargo.toml and Cargo.lock. This cleanup also resulted in the removal of numerous other unused transitive dependencies and a generalization of thiserror dependency versions across the Cargo.lock file.
Changelog
  • Cargo.lock
    • Removed entries for several unused direct and transitive dependencies, including adler2, byteorder, color_quant, core-graphics, core-text, crc32fast, dirs, dirs-sys, dlib, dwrote, fdeflate, flate2, float-ord, font-kit, freetype-sys, gif, image, jpeg-decoder, libredox, miniz_oxide, option-ext, pathfinder_geometry, pathfinder_simd, plotters-bitmap, png, redox_users, rustc_version, scheme, semver, simd-adler32, ttf-parser, weezl, wio, and yeslogic-fontconfig-sys.
    • Updated thiserror dependency specifications across multiple packages to remove explicit version numbers, allowing for more flexible version resolution.
  • Cargo.toml
    • Commented out the scheme-integration feature.
    • Commented out the scheme dependency.
  • crates/cfd-1d/Cargo.toml
    • Commented out the scheme-integration feature.
    • Commented out the scheme dependency.
  • crates/cfd-3d/src/fem/problem.rs
    • Modified the validate method to use the new get_external_boundary_nodes() function.
    • Added a new public method get_external_boundary_nodes() to retrieve nodes on the topological external boundary.
    • Added a new test case test_validate_ignores_internal_marked_faces to verify the updated validation logic.
  • crates/cfd-3d/src/fem/solver.rs
    • Uncommented the problem.validate() call in the solve method, re-enabling problem validation.
    • Updated the assemble_system method to use problem.get_external_boundary_nodes() for identifying unconstrained boundary nodes.
Activity
  • The pull request was automatically generated by Jules, indicating it's part of an automated task workflow initiated by @ryancinsight.
Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link

@recurseml recurseml bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review by RecurseML

🔍 Review performed on cc4f248..059fc0d

✨ No bugs found, your code is sparkling clean

✅ Files analyzed, no issues (5)

Cargo.lock
Cargo.toml
crates/cfd-1d/Cargo.toml
crates/cfd-3d/src/fem/problem.rs
crates/cfd-3d/src/fem/solver.rs

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request effectively resolves a validation bug in the Stokes flow problem by correctly identifying external boundary nodes. The introduction of get_external_boundary_nodes and its use in both validation and solver logic is a solid fix. The new test case, test_validate_ignores_internal_marked_faces, is well-constructed and directly verifies that the issue with internal marked faces is resolved. I've added a couple of minor suggestions to improve code style and idiomatic usage. The changes to disable the scheme dependency are also noted and seem appropriate given the description.

Comment on lines +81 to +86
let mut boundary_vertices: HashSet<usize> = HashSet::new();
for &face_idx in &external_faces {
if let Some(face) = self.mesh.face(face_idx) {
boundary_vertices.extend(&face.vertices);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This loop for collecting boundary vertices can be expressed more concisely and idiomatically using Rust's iterator methods. Using filter_map and flat_map can make the intent clearer and reduce boilerplate.

Suggested change
let mut boundary_vertices: HashSet<usize> = HashSet::new();
for &face_idx in &external_faces {
if let Some(face) = self.mesh.face(face_idx) {
boundary_vertices.extend(&face.vertices);
}
}
let boundary_vertices: std::collections::HashSet<usize> = external_faces
.iter()
.filter_map(|&face_idx| self.mesh.face(face_idx))
.flat_map(|face| &face.vertices)
.copied()
.collect();

// Apply BCs only to external nodes (0, 1, 2, 3)
// Node 4 is strictly internal.
for i in 0..4 {
boundary_conditions.insert(i, BoundaryCondition::Dirichlet { value: 0.0, component_values: None });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

There is an extra leading space on this line. Please remove it to maintain consistent formatting with the surrounding code.

Suggested change
boundary_conditions.insert(i, BoundaryCondition::Dirichlet { value: 0.0, component_values: None });
boundary_conditions.insert(i, BoundaryCondition::Dirichlet { value: 0.0, component_values: None });

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
crates/cfd-3d/src/fem/solver.rs (1)

457-468: The unconstrained-boundary-node warning is now dead code after validate() succeeds.

validate() (called at line 57) already returns Err if any external boundary node lacks a BC, so unconstrained_boundary_nodes will always be empty here. This isn't harmful—it serves as defense-in-depth if validate() is ever bypassed—but worth noting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant