Skip to content

Conversation

@aabrown100-git
Copy link
Collaborator

@aabrown100-git aabrown100-git commented Sep 28, 2025

Current situation

Resolves #444

Release Notes

  • Replace <Robin_vtp_file_path> xml element with <Spatial_values_file_path> for providing path to spatially variable Robin boundary condition .vtp file.
  • Also, add warning to uniform values RobinBoundaryCondition constructor if both stiffness and damping are 0.
  • Fix point matching bug when mesh_scale_factor is not 1.

Testing

Modifies struct/spatially_variable_robin and ustruct/spatially_variable_robin tests with new xml format.

Code of Conduct & Contributing Guidelines

@codecov
Copy link

codecov bot commented Sep 29, 2025

Codecov Report

❌ Patch coverage is 94.82759% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 67.45%. Comparing base (277dd06) to head (0f945db).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
Code/Source/solver/BoundaryCondition.cpp 93.75% 1 Missing ⚠️
Code/Source/solver/RobinBoundaryCondition.cpp 80.00% 1 Missing ⚠️
Code/Source/solver/read_files.cpp 94.11% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #446      +/-   ##
==========================================
+ Coverage   67.42%   67.45%   +0.02%     
==========================================
  Files         169      170       +1     
  Lines       34142    34173      +31     
  Branches     5727     5736       +9     
==========================================
+ Hits        23021    23050      +29     
- Misses      10983    10985       +2     
  Partials      138      138              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes issues with spatially variable Robin boundary conditions by updating XML element names and correcting point matching when mesh scaling is applied.

  • Replace deprecated <Robin_vtp_file_path> XML element with <Spatial_values_file_path> for better consistency
  • Add warning for Robin boundary conditions with both zero stiffness and damping values
  • Fix point matching bug by accounting for mesh scale factor when comparing VTP coordinates

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
tests/cases/ustruct/spatially_variable_robin/solver.xml Updates XML element name for spatial Robin BC file path
tests/cases/struct/spatially_variable_robin/solver.xml Updates XML element name for spatial Robin BC file path
Code/Source/solver/read_files.cpp Changes parameter reference from robin_vtp_file_path to spatial_values_file_path
Code/Source/solver/RobinBoundaryCondition.h Moves uniform constructor from inline to separate implementation
Code/Source/solver/RobinBoundaryCondition.cpp Implements uniform constructor with zero-value warning
Code/Source/solver/Parameters.h Removes deprecated robin_vtp_file_path parameter
Code/Source/solver/Parameters.cpp Removes deprecated robin_vtp_file_path parameter initialization
Code/Source/solver/BoundaryCondition.h Adds mesh_scale_factor parameter to find_vtp_point_index method
Code/Source/solver/BoundaryCondition.cpp Implements mesh scaling fix in point matching logic

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

…ryCondition for logging output, initialize logger earlier so that it is available when reading files.
@aabrown100-git aabrown100-git self-assigned this Oct 2, 2025
@aabrown100-git
Copy link
Collaborator Author

@ktbolt Did you want to see any other changes to this? If not, could you submit an approving review and we can merge it?

@ktbolt
Copy link
Collaborator

ktbolt commented Oct 2, 2025

@aabrown100-git I think SimulationLogger should not be embedded in BoundaryCondition.

@aabrown100-git
Copy link
Collaborator Author

aabrown100-git commented Oct 2, 2025

@ktbolt yeah I was a little reluctant to do that but I couldn't think of another way for BoundaryCondition to write to the logger from within the class. Open to ideas!

@ktbolt
Copy link
Collaborator

ktbolt commented Oct 2, 2025

@aabrown100-git I did not give much thought to logging, just wanted to have a way to write to the histor.dat file, need to give this a think so don't spend any more time on this.

Just declare logger_ as a const ref in BoundaryCondition so we know who owns it

const SimulationLogger& logger_;

@aabrown100-git
Copy link
Collaborator Author

@ktbolt I tried implementing const SimulationLogger& logger_ as suggested, but ran into some issues. Currently, the way RobinBoundaryCondition is a member of bcType and bcType is put into an std::vector, we need a default constructor for BoundaryCondition. Using a constant reference for SimulationLogger complicated this and required many changes to the code that didn't seem worth it.

Instead, I went with const SimulationLogger* logger_ because:

  1. It still shows that BoundaryCondition doesn't own the logger (it's just pointing to someone else's logger)
  2. It still prevents modification of the logger through this pointer (it's const)
  3. It required significantly fewer changes to the existing code

Let me know what you think!

@ktbolt
Copy link
Collaborator

ktbolt commented Oct 3, 2025

@aabrown100-git That'll work !

Note that std::shared_ptr is also used for this kind of thing (dependency injection).

@aabrown100-git
Copy link
Collaborator Author

aabrown100-git commented Oct 3, 2025

@ktbolt okay great! Anything else you'd like to see changed?

@ktbolt
Copy link
Collaborator

ktbolt commented Oct 3, 2025

@aabrown100-git No more changes, great work !

Adding new objects brings up some issues that we will need to address in future, very useful to get us on track.

@aabrown100-git
Copy link
Collaborator Author

@ktbolt sounds good! Could you submit an approving review, and I can merge this when the checks pass.

@aabrown100-git aabrown100-git merged commit 0305596 into SimVascular:main Oct 3, 2025
8 checks passed
@aabrown100-git aabrown100-git deleted the spatially_variable_robin_bug_fixes branch October 3, 2025 03:56
kko27 pushed a commit to kko27/svMultiPhysics that referenced this pull request Dec 17, 2025
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.

Spatially variable Robin BC bug fixes

2 participants