Skip to content

fix: accept solver stop times already reached - #344

Closed
Siel wants to merge 1 commit into
mainfrom
fix/accept-current-stop-time
Closed

fix: accept solver stop times already reached#344
Siel wants to merge 1 commit into
mainfrom
fix/accept-current-stop-time

Conversation

@Siel

@Siel Siel commented Aug 20, 2026

Copy link
Copy Markdown
Member

Summary

  • accept diffsol's StopTimeAtCurrentTime result as authoritative in both closure and DSL/JIT ODE event loops
  • preserve bolus reinitialization and continuation from infusion boundaries to later subject events
  • remove the narrower duplicate stop-time tolerance that rejected valid TSIT45 stops
  • add closure and JIT regressions using the 17-bolus/15-observation trajectory from the Pmetrics script8.R failure

Root cause

After bolus reinitialization, TSIT45 can land a few ULPs short of the requested event time. Diffsol applies its solver-specific tolerance and reports the stop as reached. The outer pharmsol event loop then requested the same stop again, received StopTimeAtCurrentTime, and rejected it with a narrower local 64-ULP check.

Diffsol already distinguishes a genuinely earlier invalid stop with StopTimeBeforeCurrentTime, so pharmsol should not second-guess StopTimeAtCurrentTime with a different tolerance.

Validation

  • cargo test — pass
  • cargo test --features dsl-jit — pass
  • cargo test --all-features — pass
  • new regression failed on v0.28.8 before the fix and passes in both closure and JIT paths after it
  • existing infusion-boundary and dense-ULP regression tests pass
  • debug/pharmsol_repro pharmsol-level examples pass:
    • script1, script2, script3
    • script6 with BDF, TSIT45, TRBDF2, and ESDIRK34
  • direct *_diffsol controls continue to reproduce their expected raw Newton failures
  • Pmetrics 3.2.5 rebuilt against this branch completes the original script8 simulation and a one-cycle fit

@Siel Siel closed this Aug 20, 2026
@Siel
Siel deleted the fix/accept-current-stop-time branch August 20, 2026 16:23
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 84.61538% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.89%. Comparing base (504b86a) to head (7bc7b6f).

Files with missing lines Patch % Lines
src/dsl/native.rs 71.42% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #344      +/-   ##
==========================================
+ Coverage   83.82%   83.89%   +0.07%     
==========================================
  Files          82       82              
  Lines       33983    33958      -25     
==========================================
+ Hits        28486    28489       +3     
+ Misses       5497     5469      -28     
Files with missing lines Coverage Δ
src/simulator/equation/ode/mod.rs 88.88% <100.00%> (+1.02%) ⬆️
src/dsl/native.rs 67.73% <71.42%> (+0.40%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions

Copy link
Copy Markdown
Contributor

🐰 Bencher Report

Projectpharmsol
Branchfix/accept-current-stop-time
Testbedmhovd-pgx

⚠️ WARNING: Truncated view!

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

🐰 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.

2 participants