Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: clade mismatch against nearest node #1526

Merged
merged 9 commits into from
Oct 29, 2024
Merged

Conversation

ivan-aksamentov
Copy link
Member

@ivan-aksamentov ivan-aksamentov commented Oct 7, 2024

Resolves #1525

After the query node placement is adjusted during the greedy tree building, sometimes the branch needs to be split and a new internal node inserted.

Currently we copy the clade of this internal node from the attachment target node. However, this is not always correct and can lead to mismatch between clade of the query node and of the new internal node.

Here I add a voting mechanism (simply a mode) between clades involved: of the parent, target and query nodes.

Copy link

vercel bot commented Oct 7, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
nextclade ✅ Ready (Inspect) Visit Preview Oct 29, 2024 8:31am

After the query node placement is adjusted during the [greedy tree building](https://docs.nextstrain.org/projects/nextclade/en/stable/user/algorithm/03-phylogenetic-placement.html#tree-building), sometimes the branch needs to be split and a new internal node inserted.

Currently we copy the clade of this internal node from the attachment target node. However, this is not always correct and can lead to mismatch between clade of the query node and of the new internal node.

Here I add a voting mechanism (simply a mode) between clades involved: of the parent, target and query nodes.
@ivan-aksamentov ivan-aksamentov changed the title add debug printouts to investigate clade mismatch against nearest node fix: clade mismatch against nearest node Oct 18, 2024
@ivan-aksamentov ivan-aksamentov marked this pull request as ready for review October 18, 2024 04:11
ivan-aksamentov added a commit that referenced this pull request Oct 18, 2024
Followup of: #1526

Additionally to moving clade let's also move "aa" label
Copy link
Member

@rneher rneher left a comment

Choose a reason for hiding this comment

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

This looks good to me. But this only works for the default clade attribute at the moment, not for the dynamics clade-like attributes.

This just extends the voting to not only clade_membership clade, but to other clade-like attributes which might be present on the tree (in `.meta.extensions.nextclade`)
@rneher
Copy link
Member

rneher commented Oct 22, 2024

when I try the query sequence linked in the issue with the RSV-A dataset, it seems that the new internal node didn't get a clade assigned for clade-like attribute G clades

image

@ivan-aksamentov ivan-aksamentov merged commit 406014e into master Oct 29, 2024
20 checks passed
@ivan-aksamentov ivan-aksamentov deleted the clade-vs-nearest-node branch October 29, 2024 08:57
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.

Assigned Clade different from Clade of assigned node
2 participants