fix: reject over-segmented skyline grids#865
Closed
ivan-aksamentov wants to merge 1 commit into
Closed
Conversation
The merger-quantile boundary construction clamps interior candidates to be non-decreasing, so requesting more segments than the tree has distinct merger times produced duplicate (zero-width) or zero-exposure segments. For those the objective I_i x - M_i ln x is unbounded below, and the solver masked it with a pooled warm-start fallback, so a per-segment Tc was set by the smoothing prior rather than the data, without a diagnostic. optimize_skyline now rejects an over-segmented grid up front (requested segments exceeding distinct merger times) and validates strictly increasing boundaries and positive per-segment exposure as backstops, erroring with an actionable message. The skyline tests use valid segment counts, assert the strictly-increasing invariant and the segment-count bound, and cover the over-segmentation error.
Member
|
let's hold off on that one. I was going to look into the regularization a bit more. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fix/coalescent-skyline-grid-validation->rustUpdate:
kb/decisions/coalescent-skyline-convex-inverse-tc.md: bound the every-segment-owns-a-merger guarantee to the supported segment count
kb/issues/N-coalescent-skyline-grid-validation-incomplete.md: record that the ordering and exposure guards are now enforced
Follow-up to: Feat/exact skyline #856
The merger-quantile boundary construction clamps interior candidates to be non-decreasing, so requesting more segments than the tree has distinct merger times produced duplicate (zero-width) or zero-exposure segments. A segment with a merger but no exposure has$M_i > 0$ and $I_i = 0$ , for which the objective $I_i x - M_i \ln x$ is unbounded below; the solver masked this with a pooled warm-start fallback, so its $T_c$ was set by the smoothing prior rather than the genealogy, with no diagnostic. The recorded guarantee that every segment owns a merger held only when the requested segment count did not exceed the distinct merger times.
This rejects an over-segmented grid up front, when the requested segments exceed the distinct merger times, and validates strictly increasing boundaries and positive per-segment exposure as backstops, each with an actionable error [src]. The skyline tests move to a tree with enough lineage-count resolution for multiple segments, assert the segment-count and ordering invariants, and cover both the over-segmentation and zero-exposure errors.
Erroring is correct because a segmentation the data cannot support has no meaningful maximum-likelihood solution: the affected segment's$T_c$ is fixed by the prior, not the data. Silently capping the segment count would change the reported model without the user's knowledge, whereas a clear error lets the user choose a supported
--n-skyline.Work items
Possible improvements