Skip to content

Handle very close ODE stop times safely - #345

Closed
Siel wants to merge 1 commit into
mainfrom
fix/ode-stop-times
Closed

Handle very close ODE stop times safely#345
Siel wants to merge 1 commit into
mainfrom
fix/ode-stop-times

Conversation

@Siel

@Siel Siel commented Aug 20, 2026

Copy link
Copy Markdown
Member

What changed

  • treat stop times within solver precision as the same time
  • restart the solver correctly after bolus and infusion changes
  • return an error if this would skip a meaningful infusion amount
  • use the same behavior for regular and DSL models

Testing

Tested all four ODE solvers, including the complete 100-cycle Pmetrics example.

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 80.41237% with 19 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.92%. Comparing base (504b86a) to head (97f52ad).

Files with missing lines Patch % Lines
src/dsl/native.rs 78.57% 9 Missing ⚠️
src/simulator/equation/ode/mod.rs 78.57% 9 Missing ⚠️
src/simulator/equation/ode/closure.rs 92.30% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #345      +/-   ##
==========================================
+ Coverage   83.82%   83.92%   +0.10%     
==========================================
  Files          82       82              
  Lines       33983    34041      +58     
==========================================
+ Hits        28486    28569      +83     
+ Misses       5497     5472      -25     
Files with missing lines Coverage Δ
src/simulator/equation/ode/closure.rs 77.31% <92.30%> (+0.60%) ⬆️
src/dsl/native.rs 68.02% <78.57%> (+0.68%) ⬆️
src/simulator/equation/ode/mod.rs 89.28% <78.57%> (+1.42%) ⬆️

... and 1 file with indirect coverage changes

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

@mhovd
mhovd requested a lite review from Copilot August 20, 2026 19:20

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 hardens the ODE event loop around solver stop times that land extremely close (within floating-point roundoff) to bolus/infusion boundaries, ensuring the logical simulation time and RHS are consistent after discontinuities across both closure-based and DSL/JIT models.

Changes:

  • Snap solver logical time to the requested stop_time when a stop is reached but the internal solver time differs by a few ULPs, triggering a safe restart when needed.
  • When StopTimeAtCurrentTime occurs, coalesce very-close forward stops after verifying the omitted infusion amount is below tolerance; otherwise return an error.
  • Add regression tests covering bolus history restarts and infusion boundary behavior across all supported ODE solvers (and compiled/JIT models behind dsl-jit).

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
tests/bolus_reinit_stop_time.rs Adds regression tests for near-stop-time discontinuities (bolus and infusion) across solvers and (optionally) DSL/JIT.
src/simulator/equation/ode/mod.rs Updates the closure-based ODE event loop to safely align time at reached stops, restart correctly, and validate coalesced infusion input.
src/simulator/equation/ode/closure.rs Introduces an infusion “amount between times” helper used to detect material skipped input when coalescing stops.
src/dsl/native.rs Mirrors the updated stop/coalescing behavior in the DSL/native ODE path and makes the compiled function call explicitly unsafe.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@github-actions

Copy link
Copy Markdown
Contributor

🐰 Bencher Report

Projectpharmsol
Branchfix/ode-stop-times
Testbedmhovd-pgx

⚠️ WARNING: Truncated view!

The full continuous benchmarking report exceeds the maximum length allowed on this platform.

🚨 3 Alerts

🐰 View full continuous benchmarking report in Bencher

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.

4 participants