Skip to content

Fix _default_dt for DelayLtiSystem (closes #544)#1054

Merged
baggepinnen merged 1 commit into
masterfrom
fix/default-dt-delay-544
May 14, 2026
Merged

Fix _default_dt for DelayLtiSystem (closes #544)#1054
baggepinnen merged 1 commit into
masterfrom
fix/default-dt-delay-544

Conversation

@baggepinnen
Copy link
Copy Markdown
Member

Summary

  • Fix _default_dt(::DelayLtiSystem) so the default time-vector dt actually adapts to the system's poles and delays. Previously the function always returned 0.07 because two […; 0] constructs collapsed r to 0, which then fell back to r = 1.0.
  • Add a regression test for Small delay with step errors #544: step(delay(0.01)) now uses dt ≈ 0.00083, ran past the delay, and produces y[end] ≈ 1.
  • Closes Small delay with step errors #544.

The new heuristic mirrors _default_dt(::LTISystem): take the fastest pole magnitude ω_max and the smallest positive delay τ_min, pick the more demanding of 1/(12·ω_max) and τ_min/12.

Context

Issue #544 was originally about step(delay(0.01)) failing with dt <= dtmin and a BoundsError. The error itself was resolved in #690 (2022) via force_dtmin=true, but the root-cause heuristic was never fixed — so step(delay(0.01)) ran without erroring while sampling far too coarsely to resolve the delay. This PR fixes that.

While verifying the fix I hit an apparently-unrelated upstream issue: MethodOfSteps(Tsit5()) + neutral DDE + SavingCallback errors out specifically at dt = 0.014 for the existing test system 1/(0.85s+1)·exp(-0.14s), while neighbouring dt values succeed. Reported as SciML/OrdinaryDiffEq.jl#3636. The new heuristic uses /12 (matching the LTI version) rather than /10, which happens to land on 0.012 instead of 0.014 and sidesteps the upstream bug for now.

Test plan

  • delay_timeresp testset passes (33/33, including the new regression test)
  • timeresp testset passes (no regressions)
  • Pre-existing step(1/(0.85s+1)*exp(-0.14s), 5) test still passes (length 417, dt 0.012)
  • step(delay(0.01)) now returns y[end] ≈ 1 with dt ≈ 0.00083

The previous heuristic always returned `0.07` because the appended `0` in
`minimum([abs.(real.(ps));0])` and `minimum([sys.Tau;0])` forced `r = 0`,
which then fell back to `r = 1.0`. As a result `step(delay(0.01))`
(issue #544) sampled at `dt = 0.07` and could not resolve the delay.

Rewritten to mirror `_default_dt(::LTISystem)`: take the fastest pole of
`sys.P.P` and the smallest positive delay, and pick the more demanding
of `1/(12·ω_max)` and `τ_min/12`. Add a regression test for #544.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@JuliaControlBot
Copy link
Copy Markdown

This is an automated message.
Plots were compared to references. 4/11 images have changed, see differences below.
After pulling this PR, please update the reference images by creating a PR to ControlExamplePlots.jl here.

Difference Reference Image New Image
✔️ 0.0 Reference New
✔️ 0.0 Reference New
✔️ 0.0 Reference New
✔️ 0.0 Reference New

@codecov
Copy link
Copy Markdown

codecov Bot commented May 14, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.57%. Comparing base (2e102ad) to head (6770392).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1054      +/-   ##
==========================================
+ Coverage   91.54%   91.57%   +0.02%     
==========================================
  Files          42       42              
  Lines        5609     5624      +15     
==========================================
+ Hits         5135     5150      +15     
  Misses        474      474              

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

@baggepinnen baggepinnen merged commit 23cefde into master May 14, 2026
6 checks passed
@baggepinnen baggepinnen deleted the fix/default-dt-delay-544 branch May 14, 2026 14:41
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.

Small delay with step errors

2 participants