Skip to content

test: add failing repro for non-monotone skyline segment boundaries#872

Draft
ivan-aksamentov wants to merge 1 commit into
refactor/unified-coalescentfrom
repro/skyline-boundary-monotonicity
Draft

test: add failing repro for non-monotone skyline segment boundaries#872
ivan-aksamentov wants to merge 1 commit into
refactor/unified-coalescentfrom
repro/skyline-boundary-monotonicity

Conversation

@ivan-aksamentov

Copy link
Copy Markdown
Member

Red counterexample for #869, base is the PR head branch so CI shows it failing against the code under review. Not intended to merge as-is: the author decides the fix, then adapts or removes the test.

merger_quantile_boundaries clamps each duplicate merger-time candidate against the previous boundary instead of coalescing merger events, so requesting more segments than there are distinct merger times yields zero-width, merger-free segments. That contradicts the docstring claim that every segment owns a merger. Zero-width segments are unreachable through segment_index, yet their per-segment log-Tc still participate in the smoothing penalty, so the fitted trajectory depends on phantom transitions.

The test requests five segments on a tree with two distinct merger times and asserts strictly increasing boundaries. On the current head it fails with the grid [2000, 2000, 2000, 2002.5, 2005, 2012]. The existing test_skyline.rs fixtures already build such over-segmented grids but assert only lengths and finiteness.

Work items

  • Assert SkylineResult.segment_boundaries is strictly increasing for an over-segmented request

Possible improvements

  • After the fix (coalesce merger groups, cap or reject unsupported n_points), add tied-merger-time and polytomy cases

…boundaries

Counterexample for #869: merger_quantile_boundaries clamps duplicate merger-time
candidates instead of coalescing them, so requesting more segments than distinct
merger times yields zero-width, merger-free segments while the docstring claims
every segment owns a merger. The test asserts strictly increasing boundaries and
fails on the current head (grid [2000, 2000, 2000, 2002.5, 2005, 2012]).

Red test for author consideration; not intended to merge as-is.
@rneher

rneher commented Jul 24, 2026

Copy link
Copy Markdown
Member

It is true that some funky things can happen if there are multiple mergers at the same time. This would be circumvented trivially if the intervals were evenly spaced. This would have pros and cons.
pro:

  • stiffness has a consistent interpretation in terms of population change in time
  • we don't have to worry about tiny intervals

cons:

  • skyline resolution is not adapted to merger patterns --> need more point to capture

I am a bit on the fence here.

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