Summary
is_at_boundary() (R/helpers.R) tests whether fitted parameters sit on their bounds with exact ==:
lower <- as.numeric(bounds$lower[names(pars)])
upper <- as.numeric(bounds$upper[names(pars)])
any(pars == lower | pars == upper)
Exact equality on doubles is fragile: after L-BFGS-B optimisation and the log/exp round-trips in the b*() bound definitions (e.g. log(range_shape1) in bburrIII3()), a parameter that is conceptually at a bound can differ from it by an ULP and fail the test. A tolerance-based comparison (e.g. abs(pars - bound) <= tol * pmax(1, abs(bound)), or isTRUE(all.equal(...)) per element) would be more robust.
Why this was deferred rather than fixed
This is behaviour-affecting in client-facing statistical code, not pure polish:
is_at_boundary() feeds the convergence / at_boundary_ok decision in ssd_fit_dists() and the boundary-driven distribution selection in ssd_fit_burrlioz().
- L-BFGS-B may legitimately return parameters clamped exactly to a bound, in which case
== is correct and intentional, and a tolerance could introduce false positives (flagging fits as at-boundary that currently are not), changing which distributions are selected/averaged and therefore changing reported results.
A decision is needed on whether the current exact comparison is intended. If a tolerance is desired, the change should be made with snapshot tests over the affected fitting paths and a check on whether any dist_data/burrlioz selections shift.
Context
Identified during a code review (the same review that produced #160–#164). Deferred deliberately as the only behaviour-affecting item.
Summary
is_at_boundary()(R/helpers.R) tests whether fitted parameters sit on their bounds with exact==:Exact equality on doubles is fragile: after L-BFGS-B optimisation and the
log/expround-trips in theb*()bound definitions (e.g.log(range_shape1)inbburrIII3()), a parameter that is conceptually at a bound can differ from it by an ULP and fail the test. A tolerance-based comparison (e.g.abs(pars - bound) <= tol * pmax(1, abs(bound)), orisTRUE(all.equal(...))per element) would be more robust.Why this was deferred rather than fixed
This is behaviour-affecting in client-facing statistical code, not pure polish:
is_at_boundary()feeds the convergence /at_boundary_okdecision inssd_fit_dists()and the boundary-driven distribution selection inssd_fit_burrlioz().==is correct and intentional, and a tolerance could introduce false positives (flagging fits as at-boundary that currently are not), changing which distributions are selected/averaged and therefore changing reported results.A decision is needed on whether the current exact comparison is intended. If a tolerance is desired, the change should be made with snapshot tests over the affected fitting paths and a check on whether any
dist_data/burrlioz selections shift.Context
Identified during a code review (the same review that produced #160–#164). Deferred deliberately as the only behaviour-affecting item.