Skip to content

fix: use shuffled child order for leaf labels in queue_shuffle - #225

Merged
Neclow merged 5 commits into
mainfrom
fix/queue-shuffle-cherries
Nov 2, 2026
Merged

Neclow merged 5 commits into
mainfrom
fix/queue-shuffle-cherries

Conversation

@bhattsamir

Copy link
Copy Markdown
Contributor

Problem

queue_shuffle(v, shuffle_cherries=True) returns an invalid label mapping most of the time. When child_order is swapped to [c2, c1], the leaf-label step still writes c2 to new_pair.1 and c1 to new_pair.0:

if child_order[1] < n_leaves {
    label_mapping[new_pair.1] = c2;   // c2 is child_order[0] after a swap
}

So the wrong child is written whenever the children were swapped. Sometimes that child is an internal node index. The mapping then either isn't a permutation of the leaves or doesn't match v_qs.

Repro (phylo2vec 1.3.1, random v with n = 5–20): 925 of 1,000 calls returned an invalid mapping. shuffle_cherries=False is unaffected, which is the only case the existing tests cover.

Fix

Read the leaf labels from child_order, the same way internal nodes are queued and coded a few lines above.

Tests

New test_queue_shuffle_shuffle_cherries (n = 3, 10, 50, 100 random vectors each) checks that, with shuffle_cherries = true:

  • v_qs is ordered
  • the mapping is a permutation of the leaves
  • cophenetic_distances(v_qs)[i][j] == cophenetic_distances(v)[map[i]][map[j]]

The test fails on main and passes with this change. cargo test -p phylo2vec passes (340 unit tests and 43 doc tests). cargo fmt --check and clippy are clean.

🤖 Generated with Claude Code

With shuffle_cherries = true, child_order can be [c2, c1], but the leaf
label mapping still assigned c2 to new_pair.1 and c1 to new_pair.0. When
the children were swapped, this wrote the wrong child (sometimes an
internal node index) into label_mapping, so the returned mapping was not a
permutation of the leaves or did not match v_qs (~92% of calls for
n = 5..20).

Read the labels from child_order, matching how internal nodes are queued
and coded. Add a regression test checking that, with shuffle_cherries,
v_qs is ordered, the mapping is a permutation, and the cophenetic
distances of v_qs match those of v under the mapping.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Neclow
Neclow self-requested a review November 2, 2026 10:25
@Neclow Neclow self-assigned this Nov 2, 2026
Neclow and others added 3 commits November 2, 2026 10:32
The assert could not detect the shuffle_cherries bug: the bad mappings
held unique (internal node) indices, so the check passed. With the fix it
cannot fire, and it built a HashSet on every call in release builds.
test_queue_shuffle_shuffle_cherries now checks the stronger properties
(mapping is a permutation of the leaves, same tree under the mapping).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Check that the new vector is ordered, the mapping is a permutation of
the leaves, and the tree is unchanged under the mapping.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Cover both values of shuffle_cherries: the new vector is ordered, the
mapping is a permutation of the leaves, and the tree is unchanged under
the mapping.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@Neclow Neclow left a comment •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the PR! Pushed three commits on top:

  • refactor: removed the label_mapping uniqueness assert in queue_shuffle. It couldn't catch this bug (the bad mappings held unique internal-node indices, so it passed), and with the fix it can't fire, so it was just building a HashSet on every call.
  • test(py): shuffle_cherries=True test in test_reorder.py.
  • test(r): queue_shuffle test in test_utils.R (there was none), covering both values of shuffle_cherries.

Comment thread phylo2vec/src/vector/ops.rs Outdated
use crate::vector::base::sample_vector;
use crate::vector::graph::cophenetic_distances;

for _ in 0..100 {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We don't usually use loops for the tests here in Rust, we let Python/R have multiple tests.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Done in ff5c66b (one sample per case, n = 5, 10, 50, 100).

Rust tests sample one tree per case; repeated random trials are left to
the Python and R suites. Use n_leaves = 5, 10, 50, 100 like the other
sampled tests in this module.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Neclow
Neclow merged commit 8915a77 into main Nov 2, 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.

2 participants