search

jangevaare / PhyloTrees.jl

Phylogenetic trees in Julia
Other
22 stars 8 forks source link

WIP: Parameterise everything... #14

Closed richardreeve closed 7 years ago

richardreeve commented 7 years ago

@jangevaare, in response to #13, you might reasonably think this is a bit much(!), but I've completely rewritten the code to allow for a variety of parameterisation types without impacting the base cost of creating an unparameterised tree (hopefully!), and without significantly changing your API...

See what you think...

codecov-io commented 7 years ago

Codecov Report

:exclamation: No coverage uploaded for pull request base (master@aa56963). Click here to learn what that means. The diff coverage is 43.55%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #14   +/-   ##
=========================================
  Coverage          ?   43.55%           
=========================================
  Files             ?        8           
  Lines             ?      535           
  Branches          ?        0           
=========================================
  Hits              ?      233           
  Misses            ?      302           
  Partials          ?        0
Impacted Files Coverage Δ
src/plot.jl 0% <0%> (ø)
src/show.jl 0% <0%> (ø)
src/traversal.jl 0% <0%> (ø)
src/interface.jl 49.72% <49.72%> (ø)
src/utilities.jl 52.63% <52.63%> (ø)
src/distance.jl 57.14% <57.14%> (ø)
src/trees.jl 67.64% <67.64%> (ø)
src/structure.jl 80% <80%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update aa56963...fcaae00. Read the comment docs.

richardreeve commented 7 years ago

Things still to do:

jangevaare commented 7 years ago

Can we agree on what leaves are? Does a node have to have a connection to a root to be a leaf, or is an unconnected node a leaf too?

I've been referring to leaf nodes as nodes which have an in degree of 1 and an out degree of 0

Can we agree on what a node is? In some places the original api refers to everything as nodes, and elsewhere only internal (non-leaf, non-root, non-unconnected) as nodes... I much prefer the former...

Everything as nodes was the intention!

jangevaare commented 7 years ago

you might reasonably think this is a bit much(!) This is a lot, yes. It will take me awhile for me to review (once you've finished). I can't guarantee I will merge this PR, but at the very least it provides a basis for many important discussions on how the package can be improved - thank you for that.

This would all be more digestible if each feature addition were a PR, but I understand that's not always feasible due to speed, interdependencies, etc.

Anyways, I'll keep an eye on this. Ping me when you think it's reasonably complete!

richardreeve commented 7 years ago

So, as far as "everything as nodes" is concerned, you have isnode(), findnodes() and findnonnodes(), which disagrees. I would love to change those names to isinternal(), and so on if you agree?

jangevaare commented 7 years ago

So, as far as "everything as nodes" is concerned, you have isnode(), findnodes() and findnonnodes(), which disagrees. I would love to change those names to isinternal(), and so on if you agree?

Yes, you are right, that's what they should be. Can we do that in a separate pull request though? (Or I can quickly do it myself)

richardreeve commented 7 years ago

As far as your more general comment is concerned, I can understand what you mean, but what I really wanted to do was add an abstraction above your tree, and then add another implementation to demonstrate why that was useful for us. It's difficult to do that without making a lot of edits, and in the end I just did the whole thing. It's basically what we were thinking of doing a while ago with #11, but it turned out not to be necessary then. I've tried to keep your interface completely, but then add in another full implementation of everything in parallel.

I'm completely happy to introduce a fourth Phylo... implementation into Julia because I understand that we need these features and you don't, but it seemed wasteful. What I would prefer though was either:

Anyway, the code is complete from our perspective, there's just some bugfixes going in now, so go ahead and look.

I'm going to go ahead and implement Phylogenetic diversity measures on top of it. It may currently not be as computationally efficient as I'd like (I don't know, I haven't profiled it), but I don't think there's anything in it that should make that impossible - it's just practical issues.

richardreeve commented 7 years ago

Just seen your text about the separate PR - please don't do that now. isnode() is just a const copy of isinternal() anyway in my code (with a view to dropping it if you agreed). I know you can easily make that one fix, but I've changed so much of your code, it's essentially irrelevant and I assume that no-one is relying on it for the time being anyway...

jangevaare commented 7 years ago

Just seen your text about the separate PR - please don't do that now. isnode() is just a const copy of isinternal() anyway in my code (with a view to dropping it if you agreed). I know you can easily make that one fix, but I've changed so much of your code, it's essentially irrelevant and I assume that no-one is relying on it for the time being anyway...

Okay, hadn't realized this was already implemented @richardreeve . Going through some of this work now.

richardreeve commented 7 years ago

I've finished replacing is|find[non]node[s] with *internal[s] in the rest of the interface... sorry, I couldn't help myself :)

richardreeve commented 7 years ago

btw, as far as leaves are concerned, I don't feel very strongly about it. Is there a formal definition of what a leaf of a tree is? I could go with not including them, but I'm just not sure why it matters whether you have an in degree of 1 (obviously I agree that an out degree of 0 is essential!).

richardreeve commented 7 years ago

@jangevaare Okay, my basic phylogenetic diversity code now all seems to work - it's in richardreeve/Diversity.jl#18 if you're interested (you can just check out the phylogenetics branch of the package) - so I'm wondering how to proceed. Do you think you'd be more comfortable / more likely to go along with:

Or should I just create my own phylogenetics package? It doesn't make a massive difference to me at this point. I can reuse much of what I've done here...

jangevaare commented 7 years ago

@richardreeve so I have done a once through your code. I'm on board with having an AbstractTree, still considering many of the other additions though, bear with me, you've given me lots to think about!

richardreeve commented 7 years ago

@jangevaare Thanks for your comments. I'm actually thinking about the complexity of having an AbstractPhylo package / git repository, and it doesn't seem too bad, so I don't mind if you prefer that, but there is quite a lot of functionality that I do need. Happy to have an actual (voice!) chat if you feel like it...

richardreeve commented 7 years ago

It's okay. I can understand I'm proposing big changes to your code. I've decided to take a different approach.

jangevaare commented 7 years ago

Hey @richardreeve sorry I left you hanging there. Partly because I don't have the resources to maintain Phylo.jl with any broader of a scope right now. I applaud your initiative and will keep my eye on your new package (as well as the BioJulia/Phylogenies package)

richardreeve commented 7 years ago

@jangevaare No problem. It may turn out to be a lot of work (or BioJulia/Phylogenies may turn out to be the long term solution, which would be ideal, but it's not really there yet).