Skip to content

Replace skipped sample-as-parent failure test for variational_gamma with a passing test - #520

Merged
hyanwong merged 2 commits into
mainfrom
copilot/fix-sample-as-parent-variational-gamma
Oct 28, 2026
Merged

hyanwong merged 2 commits into
mainfrom
copilot/fix-sample-as-parent-variational-gamma

Conversation

Copilot AI commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

A test for variational_gamma was skipped pending a decision on whether it should raise NotImplementedError when a sample has undated descendant nodes. The EP machinery turns out to handle this case already, so no error is needed.

  • Test change only (tests/test_functions.py)
    • Removed the skipped test_variational_gamma_sample_as_parent_fails.
    • Added test_variational_gamma_sample_as_parent. It adds a mutation to single_tree_ts_n3_sample_as_parent(), because the fixture has none and variational_gamma requires mutations. It then checks that sample ages are unchanged after dating.
  • No library change
    • A guard that raised on samples-as-parents broke TestInternalConstraints::test_with_unary_internal_samples and test_with_nonunary_internal_samples. Both already date internal samples through date().
    • A guard on non-time-0 samples broke the same two tests, so non-contemporary samples are also supported. I reverted both guards.
ts = single_tree_ts_n3_sample_as_parent()  # sample 3 is the parent of samples 0 and 1
dts = tsdate.variational_gamma(ts, mutation_rate=1, rescaling_intervals=1)
# nodes_time: [0, 0, 0, 1, 1.00000001]; sample 3 stays at 1

Review notes:

  • The new test passes rescaling_intervals=1. With the default, this three-sample fixture fails with AssertionError: Use fewer rescaling intervals. That is a limit of piecewise rescaling on a tiny tree, not a sample-as-parent problem, and I didn't change it.
  • The coverage is a single three-sample tree with one mutation. It checks that sample ages are preserved, not the quality of the inferred ages for the non-sample nodes.

Copilot AI linked an issue Oct 28, 2026 that may be closed by this pull request
Co-authored-by: hyanwong <4699014+hyanwong@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix sample_as_parent handling in variational_gamma implementation Replace skipped sample-as-parent failure test for variational_gamma with a passing test Oct 28, 2026
Copilot AI requested a review from hyanwong October 28, 2026 16:29
@hyanwong
hyanwong marked this pull request as ready for review October 28, 2026 17:25
@hyanwong

Copy link
Copy Markdown
Member

This LGTM. The functionality was added in #433 (there is a lot of discussion and some more test cases there)

@hyanwong hyanwong left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This just reactivates a test that should now pass, and is uncontroversial

@hyanwong
hyanwong merged commit 43aa088 into main Oct 28, 2026
9 checks passed
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.

sample_as_parent in variational_gamma

2 participants