Skip to content

Conversation

@aabrown100-git
Copy link
Collaborator

@aabrown100-git aabrown100-git commented Oct 30, 2025

Current situation

#464

Release Notes

  • Adds option for user to provide a conversion factor for pressure and flowrate running an svMultiPhysics/sv0D coupled simulation

Testing

  • Added new test case pipe_RCR_sv0D_different_units, which is identical to pipe_RCR_sv0D but uses mmhg and mL for sv0D parameter file. Note that the reference solution for pipe_RCR_sv0D_different_units is identical (in fact the same file) as pipe_RCR_sv0D.

Code of Conduct & Contributing Guidelines

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 adds support for unit conversion between 3D (svMultiPhysics) and 0D (svZeroD) coupled simulations. The implementation allows automatic conversion of pressure and flowrate values between different unit systems (e.g., cgs vs. clinical units).

  • Adds optional Pressure_conversion_factor and Flowrate_conversion_factor parameters to svZeroDSolver_interface XML configuration
  • Implements bidirectional unit conversions: multiply when sending values to 0D model, divide when receiving from 0D model
  • Adds test case pipe_RCR_sv0D_different_units to validate mixed-unit coupling (cgs in 3D, mmHg/mL in 0D)

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
Code/Source/solver/Parameters.h Declares new conversion factor parameters in svZeroDSolverInterfaceParameters
Code/Source/solver/Parameters.cpp Initializes conversion factor parameters with default value of 1.0
Code/Source/solver/ComMod.h Adds conversion factor fields to svZeroDSolverInterfaceType
Code/Source/solver/ComMod.cpp Implements logic to copy conversion factors from parameters to runtime type
Code/Source/solver/svZeroD_subroutines.cpp Applies conversion factors when exchanging values between 3D and 0D solvers
tests/test_fluid.py Adds test function for mixed-units case
tests/cases/fluid/pipe_RCR_sv0D_different_units/solver.xml Solver configuration with cgs units and conversion factors
tests/cases/fluid/pipe_RCR_sv0D_different_units/svzerod_3Dcoupling.json 0D model configuration with clinical units
tests/cases/fluid/pipe_RCR_sv0D_different_units/README.md Documentation for the mixed-units test case
tests/cases/fluid/pipe_RCR_sv0D/svzerod_3Dcoupling.json Adds comment documenting original test uses cgs units

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

for (int s = 0; s < numCoupledSrfs; ++s) {
if (init_flow_flag == 1) {
lpn_state_y[sol_IDs[2 * s]] = init_flow;
// Apply flowrate conversion when initializing state
Copy link

Copilot AI Oct 30, 2025

Choose a reason for hiding this comment

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

The comment indicates conversion is applied when initializing state, but doesn't explain the direction. The README states the conversion factor transforms 3D units to 0D units (e.g., dyn/cm² → mmHg), but init_flow is a 3D value being stored in lpn_state_y (0D state). Consider clarifying whether init_flow should be in 3D or 0D units, and if the conversion direction is correct here.

Suggested change
// Apply flowrate conversion when initializing state
// Convert init_flow from 3D units to 0D units when initializing state

Copilot uses AI. Check for mistakes.
}
if (init_press_flag == 1) {
lpn_state_y[sol_IDs[2 * s + 1]] = init_press;
// Apply pressure conversion when initializing state
Copy link

Copilot AI Oct 30, 2025

Choose a reason for hiding this comment

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

Similar to the flowrate initialization, the comment doesn't clarify the direction of conversion. Consider adding details about whether init_press is expected to be in 3D or 0D units, and confirming the multiplication is the correct operation for this initialization path.

Suggested change
// Apply pressure conversion when initializing state
// Apply pressure conversion when initializing state.
// init_press is expected to be in 3D units (e.g., dyn/cm^2 or mmHg).
// The solver expects pressure in 0D units.
// Multiplying by solver_interface.pressure_conversion converts from 3D to 0D units.

Copilot uses AI. Check for mistakes.
@codecov
Copy link

codecov bot commented Oct 30, 2025

Codecov Report

❌ Patch coverage is 78.57143% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 67.35%. Comparing base (4db1403) to head (8241336).

Files with missing lines Patch % Lines
Code/Source/solver/svZeroD_subroutines.cpp 62.50% 3 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #465   +/-   ##
=======================================
  Coverage   67.35%   67.35%           
=======================================
  Files         169      169           
  Lines       32620    32626    +6     
  Branches     5718     5720    +2     
=======================================
+ Hits        21970    21976    +6     
  Misses      10513    10513           
  Partials      137      137           

☔ 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
Collaborator

@ktbolt ktbolt left a comment

Choose a reason for hiding this comment

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

I do not approve of these changes.

@aabrown100-git
Copy link
Collaborator Author

Closing without merging. See issue for discussion.

@aabrown100-git aabrown100-git deleted the svZeroD_unit_conversion_factor branch November 3, 2025 21:34
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.

2 participants