legumeinfo / tripal_phylotree

LIS project- tripal module for chado phylogeny and gene families
GNU General Public License v2.0
1 stars 7 forks source link

fail to lookup intergenic distance for tree #15

Closed guidorice closed 7 years ago

guidorice commented 7 years ago

On many trees, such as: phytozome_10_2.59027447 the tnt.tree fails to compute the distance for the scale bar, and so no scale bar is shown. I am not sure if this is because of a data loading problem into chado.phylonode or a problem in the javascript code.

@adf-ncgr do you have any insights into this?

adf-ncgr commented 7 years ago

I don't think it is a data loading problem- the tree for phytozome_10_2.59027447 appears to have a normal scale bar on the production server using the older code, and if on the dev server you ask for the newick of that tree from the root node, the lengths are there and I don't notice any abnormality (which doesn't mean there isn't one). I assume you've seen the javascript error in the console? (maybe this is just saying it couldn't get the branch lengths further upstream): tree.js:255failed to lookup intergenic distance for tree:init @ tree.js:255 tree.js:256Objectobservers: Object_id: 1_root_dist: 0branch_length: 0children: Array[2]cvterm_name: "phylo_root"depth: 0length: 0name: ""parent_phylonode_id: 0phylonode_id: (...)get phylonode_id: ()set phylonode_id: ()phylotree_name: "phytozome_10_2.59027447"textAnchor: "start"value: 3.99268x: 216.09375y: 0proto: Object

guidorice commented 7 years ago

Yes, the error is where my code gets undefined back from tnt.tree api for the distance calculation. Then it just prints the tree data, just in case anything interesting could be seen. It may not like zero length branches, not sure but that is what I suspect.

adf-ncgr commented 7 years ago

was just looking at it again, one thing I noticed that might be a clue (or a red herring) is that in the newick for phytozome_10_2.59027447 I see one zero-length branch that appears in a kind of unusual place (internal in the tree rather than at the root), though I am not sure it is technically wrong. but I think you are probably on the right track with your last comment. Will check the source tree and make sure what we see in the newick matches the original file.

adf-ncgr commented 7 years ago

looks like the "):0," that I saw in the viewer-generated newick is consistent with what was in the original tree file, except all the decimal places were listed in the latter:

araip.Araip.SB6R7:0.011150):0.118340):0.041320):0.060710**):0.000000,** (vitvi.GSVIVT01025184001

so, I guess you could perhaps try modifying 0s to .0000001 (or similar) when loading in and see if that makes the problem go away (or we could do this at the db level, maybe that would be easier- as long as we use something that we can revert easily if it doesn't help)

guidorice commented 7 years ago

ok sounds like worth a try. here is the code where it calculates the distance. no fixes are jumping out at me (dont really understand the code). I guess one of the multiplication steps is causing one term to get zeroed out.

https://github.com/tntvis/tnt.tree/blob/master/src/tree.js#L411

guidorice commented 7 years ago

Added an issue over here https://github.com/tntvis/tnt.tree/issues/7

guidorice commented 7 years ago

Setting 0's to .0000001 seems to be a good workaround (the scale bar renders in this case). Although in the newick export it shows up as 1e-7 (scientific). From what I have read, that is not technically wrong, but it may cause problems with other newick parsers. Thoughts?

adf-ncgr commented 7 years ago

I suspect that it will depend on the parser as to whether or not this causes problems. I'd say it's fine to use as a workaround for now and we can see what the tntvis folks say about it. It is admittedly a little weird to have zero-length branches internally rather than treating it as a node with more than two children, although it's pretty typical to assume that phylogenetic trees are strictly bifurcating.

guidorice commented 7 years ago

The tnt.tree author released a fix for this already, so I will get it into the release.