search

veg / phylotree.js

Interactive viewer of phylogenetic trees
http://phylotree.hyphy.org
MIT License
169 stars 77 forks source link

getNewick() returns unquoted Newick strings #438

Open gaurav opened 1 year ago

gaurav commented 1 year ago

I've been trying to use tree.getNewick() to implement a custom menu item in phylotree.js v1.4.0 that can be used to rename a node. The algorithm I use is:

  1. The custom menu item is added to every node in the phylogeny, and asks the user for a new name for that node.
  2. It then sets that node's node.name property to the new string, surrounding it in double-quotes in case the user has included whitespace. This updates the internal Newick representation of the phylogeny.
  3. It then uses tree.getNewick() to retrieve the complete Newick representation of the phylogeny, and redraws the tree with the new Newick string.

This seems to work well for Newick strings that don't have any whitespace. However, when my Newick string does have whitespace, tree.getNewick() returns a string like:

(Tomistoma schlegelii:1,(Osteolaemus tetraspis:1,Crocodylus niloticus:1,"Test test":1)Crocodylinae:1);

i.e. only the name that I've explicitly quoted has quote characters, and other names -- even those containing spaces -- are loaded unquoted. This is incorrect Newick, which requires quoting with single quoting for names containing strings (see Wikipedia and the Newick spec). When loading this new Newick string, phylotree.js (correctly) ignores spaces, so "Crocodylus niloticus" is rendered as Crocodylusniloticus.

I think the issue is in the following line of code, which doesn't quote names as they are exported:

https://github.com/veg/phylotree.js/blob/ca174f3f3af9bc668021ff827da7e01cc41c1706/src/formats/newick.js#L253

Could you please modify this line of code so that names being exported are properly quoted? As per the spec, names should be quoted with single quotes before and after the name, and any single quotes present within the name should be doubled to escape it (i.e. ab'de should be written as 'ab''de')

gaurav commented 1 year ago

I took a stab at fixing this in PR https://github.com/veg/phylotree.js/pull/441/files -- please let me know if this is useful for you, and if you have any suggestions for improvement.

gaurav commented 11 months ago

Thanks so much for improving and merging PR https://github.com/veg/phylotree.js/pull/441! Do you know when there might be a release containing that change? I tried using phylotree.js directly from GitHub (using npm install --save https://github.com/veg/phylotree.js), but that doesn't seem to include the CSS files, so I don't think I can test this issue until I can install phylotree from npm.

stevenweaver commented 11 months ago

Thanks @gaurav for your contribution. I've released v1.4.1 which should include changes from #441.

gaurav commented 10 months ago

Thank you, Steven! This upgrade has fixed most of our node renaming issues. There's still a minor bug when renaming a node to a name containing a single quote (see https://github.com/phyloref/klados/pull/312); I'll continue poking at it and see if it's being caused in my software or in phylotree.js.