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

High performance newick parsing. #2187

Open
5 tasks
benjeffery opened this issue Apr 6, 2022 · 12 comments
Open
5 tasks

High performance newick parsing. #2187

benjeffery opened this issue Apr 6, 2022 · 12 comments
Labels
enhancement New feature or request Python API Issue is about the Python API
Milestone

Comments

@benjeffery
Copy link
Member

benjeffery commented Apr 6, 2022

Our current newick parsing relies on the newick python library, which for trees we've tested is ~120x slower than parsing with a common (C extension) R library. My initial rough experiments with a pure-python one-pass pre-allocated state machine in tskit-dev/tsconvert#36 are very promising, timing at 1-2x the R library. A C implementation could follow if we see the benefit and the Python implementation is amenable to conversion.

Requirements:

  • Parses labels storing these in metadata. (Open question, should labels go into node-associated individuals?)
  • Parse comments storing in node metadata.
  • Parse branch lengths and post-process these to node times taking care to handle the numerical precision issues therein.
  • Support files with multiple roots
  • Support for unary nodes

Support for unicode is not required.

@benjeffery benjeffery added enhancement New feature or request Python API Issue is about the Python API labels Apr 6, 2022
@benjeffery benjeffery added this to the Python 0.6.0 milestone Apr 6, 2022
@jeromekelleher
Copy link
Member

Also, support for unary nodes would be good (but I suspect this will happen automatically)

@benjeffery
Copy link
Member Author

Also, support for unary nodes would be good (but I suspect this will happen automatically)

Yes, thanks! Added to the list. @hyanwong @szhan do you have anything to add?

@hyanwong
Copy link
Member

hyanwong commented Apr 6, 2022

I made this point somewhere else, but the ability to parse into a table collection would mean that we could cope with zero-length (or even negative-length) branches.

Also, I'm not sure if we want to worry about trees beginning with [&U] and [&R]. We only "do" rooted trees in tskit, AFAIK.

@benjeffery benjeffery modified the milestones: Python 0.5.2, Python 0.5.1 Apr 6, 2022
@benjeffery
Copy link
Member Author

I made this point somewhere else, but the ability to parse into a table collection would mean that we could cope with zero-length (or even negative-length) branches.

What would one then do with the table collection? There's currently no way to get to a Tree without satisfying tskit's node time constraints, right?

@hyanwong
Copy link
Member

hyanwong commented Apr 6, 2022

What would one then do with the table collection?

I would run through the node times and adjust them to make a valid TS, while adding stuff to the metadata to say what I had done.

@benjeffery
Copy link
Member Author

What would one then do with the table collection?

I would run through the node times and adjust them to make a valid TS, while adding stuff to the metadata to say what I had done.

Great - was checking we didn't need to add a way to get to the quintuple array representation without the integrity constraints. Now you say it I remember you mentioning the fixing-and-recording.

@hyanwong
Copy link
Member

hyanwong commented Apr 8, 2022

It should be easy to find the "bad" nodes by doing

bad_edges = np.where(tables.nodes.time[tables.edges.parent] <= tables.nodes.time[tables.edges.child])[0]

right? No need to traverse the tree. Fixing them might be more difficult, however, although zero length branches can probably be fixed by adding an epsilon or using last after, as we do in split_polytomies

@benjeffery
Copy link
Member Author

Yes good idea - I think that fixing non-conformant tree sequences sounds like a job for a separate function/set of functions though and not part of this work.

@hyanwong
Copy link
Member

hyanwong commented Apr 8, 2022

Yes, 100%

@hyanwong
Copy link
Member

hyanwong commented Nov 1, 2022

Incidentally, what is the current status of tskit's Newick parsing capabilities, @benjeffery? Do we have (relatively) fast parsing in a released version yet?

@benjeffery
Copy link
Member Author

Still a proof of concept I'm afraid! It's on the medium term Todo list...

@hyanwong
Copy link
Member

hyanwong commented Nov 2, 2022

A quick additional note here: I wonder if we should allow nodes without branch lengths in the Newick file, and then set the time_units to uncalibrated. We could have a parameter from_newick(missing_length=X) which specifies the value to use if the branch length is missing. Perhaps if None we error out on missing branch lengths. missing_length=0 could also be allowed, to create an invalid tree sequence but a valid set of tables (see tskit-dev/tsconvert#36 (comment)).

This would also be useful for the "introduction to tskit for phylogeneticists" tutorial (tskit-dev/tutorials#163)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Python API Issue is about the Python API
Projects
None yet
Development

No branches or pull requests

3 participants