search

bokulich-lab / q2-coordinates

methods for geographic mapping of qiime2 artifact data or metadata
BSD 3-Clause "New" or "Revised" License
2 stars 8 forks source link

Do quadtrees need a new transformer? #13

Closed nbokulich closed 3 years ago

nbokulich commented 3 years ago

This is a followup question/issue related to #10

I note that the quadtree is output as a Phylogeny[Rooted], but the README shows some provenance-breaking reformatting necessary to use with q2-empress. Is a new transformer needed? Or why is this reformatting not just done in the first place?

If reformatting is necessary for this to be used as a normal Phylogeny[Rooted], then a quadtree should have its own special type (maybe Phylogeny[quadtree]), and we can add a transformer or an action to perform this conversion.

Any thoughts @zoechallacombe @wasade ? Thanks!

wasade commented 3 years ago

I think that step is no longer needed as empress should be able to handle edges w/o a specified branch length. The other option is to just explicitly set the edges to 0.0 during construction. I do not think a separate type is necessary.

zoechallacombe commented 3 years ago

On the newest release of qiime2 empress still gives the error "At least one non-root branch of the tree must have a positive length." So setting the edges to some value is still necessary. This could be because nothing in the tree has a specified branch length, but branch length for this application is pretty arbitrary. Working on explicitly setting the node lengths to 0.0 during construction. I also don't think a separate type is necessary.

nbokulich commented 3 years ago

Working on explicitly setting the node lengths to 0.0 during construction

Thanks @zoechallacombe ! This would be ideal!

zoechallacombe commented 3 years ago

Fixed and created a new pull request to merge changes.