Skip to content

fix: harden skyline solver inputs and convergence#864

Closed
ivan-aksamentov wants to merge 1 commit into
rustfrom
fix/coalescent-skyline-solver-robustness
Closed

fix: harden skyline solver inputs and convergence#864
ivan-aksamentov wants to merge 1 commit into
rustfrom
fix/coalescent-skyline-solver-robustness

Conversation

@ivan-aksamentov

Copy link
Copy Markdown
Member

fix/coalescent-skyline-solver-robustness -> rust

The skyline optimizer had three robustness defects. Merger times were sorted with partial_cmp().expect(), which panics on a NaN time and departs from the project's OrderedFloat convention. A non-positive or non-finite stiffness was silently treated as no smoothing although the parameter is documented positive. And the Newton loop returned its last iterate whether or not the gradient tolerance was met, so a non-converged fit was indistinguishable from a converged one.

This sorts merger times with OrderedFloat and rejects non-finite times, rejects non-finite or non-positive stiffness, non-finite tolerance, and a zero iteration budget for multi-segment fits, and warns when Newton returns without reaching the gradient tolerance [src].

The input guards are hard errors because a non-finite or non-positive smoothing parameter has no valid interpretation and signals misconfiguration. Non-convergence is a warning rather than an error because the last iterate is still a usable estimate, and failing the run would discard an otherwise-progressing fit.

Work items

  • Sort merger times with OrderedFloat and reject non-finite times [src]
  • Reject non-finite/non-positive stiffness, non-finite tolerance, and zero max-iter for multi-segment fits [src]
  • Warn when the Newton solve does not reach the gradient tolerance [src]
  • Add tests for the stiffness guards [src]

Three robustness holes in the skyline optimizer: merger times were sorted with partial_cmp().expect(), which panics on a NaN time and departs from the OrderedFloat convention; a non-positive or non-finite stiffness was silently treated as no smoothing although the parameter is documented positive; and the Newton loop returned its last iterate whether or not the gradient tolerance was met, so a non-converged fit was indistinguishable from a converged one.

Sort merger times with OrderedFloat and reject non-finite merger times; reject non-finite or non-positive stiffness, non-finite tolerance, and zero max_iter for multi-segment fits; and warn when Newton returns without reaching the gradient tolerance.
@rneher

rneher commented Jul 23, 2026

Copy link
Copy Markdown
Member

this one makes sense. But interacts with the idea of folding the constant opt into the skyline.

@ivan-aksamentov
ivan-aksamentov deleted the fix/coalescent-skyline-solver-robustness branch July 24, 2026 15:25
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