OpenTreeOfLife / opentree

Opentree browsing and curation web site. For overarching or cross-repo concerns, please see the 'germinator' repo.
http://tree.opentreeoflife.org/
BSD 2-Clause "Simplified" License
109 stars 26 forks source link

Unable to view and edit tree in curation app #349

Closed chris-owen closed 10 years ago

chris-owen commented 10 years ago

I uploaded a study, Jørgen Lützen, 2003, and the tree does not appear when I click the file to view it (I want to root it and designate the ingroup). No errors appear when I upload the file as a newick or nexus or paste it in the box as a newick or nexus. Also, I am able to map the taxa so I assume the app can read the file. The newick string with bootstrap values is below. Thanks for your help!

(((Sacculina_sinensis,((Sacculina_oblonga,Sacculina_confragosa)100,Sacculina_leptodiae)78)100,((Polyascus_polygenia,Polyascus_plana)54,Polyascus_gregaria)100)100,(Loxothylacus_panopaei,(Heterosaccus_californicus,Sacculina_carcini)48)99,Boschmaella_japonica);

jimallman commented 10 years ago

Thanks for the detailed report, @chris-owen. It seems something is going wrong when we import + merge the OTUs for this tree.

@mtholder, I've tested importing the Newick above. It looks like we're getting weird output from to_nexson, with nodes reporting a wider range of OTU ids than are returned in the otus collection. This is not corrected in merge_otus, so the tree viewer eventually fails when it can't find a specified OTU.

mtholder commented 10 years ago

NCL bug. I'll try to fix it today. It looks like the importer is being inconsistent wrt how it deals with internal node labels. They are being treated as OTUs in the tree, but in the otus element it is thinking of them as arbitrary text (not otus). So they are not being emitted as OTUs in the otus element of NeXML

jimallman commented 10 years ago

Once we know the likely decisions (interpretations) for these mystery values, the curation app will re-tag them to something sensible.

jimallman commented 10 years ago

Also check any fix against this tree (re-import and test?) from @josephwb

http://devtree.opentreeoflife.org/curator/study/view/ot_41/?tab=trees&tree=tree1

mtholder commented 10 years ago

There is a pull request to fix the server side of this problem. https://github.com/OpenTreeOfLife/opentree/pull/350 If that gets deployed, then the tree should come in OK.

Ideally the JS code in the curator would check for '@label' fields in the internal nodes of the tree. If they are found, then the user should be prompted to clarify what the values mean (because it is not possible to determine the meaning from the newick content). UI choice should be something like (note that the support statements are move to the egde that has the node as its target):

user shown edge predicate transformation
Bootstrap proportion (0-1) ot:bootstrapSupport convert to double and multiply by 100
Bootstrap percentage (0-100) ot:bootstrapSupport convert to double
Posterior prob (0-1) ot:posteriorSupport convert to double
Posterior percentage (0-100) ot:posteriorSupport convert to double and divide by 100
Other support ot:otherSupport and ot:otherSupportType store value of @label in ot:otherSupport and free text description in ot:otherSupportType
Not a support statement Leave in @label
jimallman commented 10 years ago

@mtholder : Should I consider a @label on an internal node as an error? If so, I can always prompt for the choices above in any tree, whether imported on-the-fly or found in an existing study.

mtholder commented 10 years ago

well it is not necessarily an error (newick, nexus, and nexml all allow for arbitrary labels). Unfortunately, lots of folks use the label for storing support info. We might want a tree-level property named something like '^ot:labelMeaning' If a tree is seen with internal node labels, but that tree-level property is NULL, then we could visually indicate a warning. If the labels are one of the support values listed above, we could move the info to the more precise field. for the last possibility in the table ("not a support statement"), we could set "^ot:labelMeaning" to something (even "arbitrary") so that the user is not continually nagged about this if the tree does have arbitrary labels. The gotcha is that if the tree is rerooted (if the current root is not the original root) then moving the node label to the (more precise) edge property won't work. So we may only want to do the coercion/moving as a part of the import process. The flexible/correct way to do it would support coercing the labels to edge properties at any time, but would perform a hidden rerooting procedure (to restore the original root temporarily) so that the correct edges get decorated. That sounds like a real pain; so I'd be inclined to keep this as an "on import" only user interaction.

jimallman commented 10 years ago

well it is not necessarily an error (newick, nexus, and nexml all allow for arbitrary labels).

Agreed, "error" was too strong a word. For our purposes, it's noteworthy, maybe enough to add a prompt to the user interface.

The gotcha is that if the tree is rerooted (if the current root is not the original root) then moving the node label to the (more precise) edge property won't work. So we may only want to do the coercion/moving as a part of the import process.

Hm, food for thought. I'll start there and see how it behaves..

jimallman commented 10 years ago

Unfortunately, lots of folks use the label for storing support info. We might want a tree-level property named something like '^ot:labelMeaning'

Hm, I think this already exists! An older email from @josephwb pointed to ot:nodeLabelMode in the NexSON wiki page, with these values:

@mtholder, @josephwb : Does this sound right to you? I'm coding the JS based on this understanding, but let me know if I should go back to using ot:labelMeaning (and if so, what values are expected there).

jimallman commented 10 years ago

Hm, I see that @mtholder's table of values above should probably supercede the values in the NexSON wiki for ot:nodeLabelMode. Should I update the wiki accordingly? Or am I missing a distinction between these two?

jimallman commented 10 years ago

@mtholder, @josephwb : Based on my best understanding, I've implemented a pretty complete UI for qualifying and "freezing" internal labels. This is available for review on devtree, and it should work with any new or existing trees with existing labels. Comments welcome!