Repository navigation
biojava-phylo module extension #346
Description
I would like to make some changes to the phylo module of biojava. I would like to factor out the pairwise distance calculation in TreeConstructor.java to include easily more scoring functions for the evolutionary distance. I have in mind to include a structural similarity score, if the alignment comes from a structural alignment (some literature claims that the resulting tree is more accurate and has less uncertainty). Currently percentage of identity (PID) and substitution matrix scores are implemented.
I have also noticed that some code was ported from Jalview and is either not used or redundant with other biojava code. An example is the ResidueProperties.java class, that contains hard coded substitution matrices (PAM250, BLOSUM62) and a lot of color assignments and options never used in biojava (I suppose they are used for the Jalview GUI). About the matrices, the biojava-alignment module contains already these two substitution matrices in the resources folder, and many more, but the phylo module does not depend on the alignment module, so they cannot be accessed. I think it would be good to add the alignment module as a dependency of the phylo module, since generating sequence alignments is a previous step to the phylogenetic tree construction.
Lastly, some names are confusing in the module. For example, the TreeConstructionAlgorithm.java enum has BLOSUM and PID as its fields. By tree constructor, one understands one of the algorithms to build trees, like UPGMA or Neighbor Joining (NJ). Those are named in the enum TreeType.java, which should stand for one of the three tree types (Distance Tree, Maximum-Likelihood Tree or Pasimony Tree). The BLOSUM and PID fields should be grouped into something named like ScoreMatrixType to be consistent with the literature.
Does anyone have comments on (or oppose to) these changes?
Is there any important point I am missing?
Aleix
Activity
josemduarte commentedon Nov 11, 2015
Regarding dependencies: I agree that biojava-phylo should depend on biojava-alignment. Also if it is clear that the Jalview code is not used anywhere, I'd vote to remove it to avoid confusion.
Another note about substitution matrices: they are at the moment in biojava-alignment and I definitely think they should stay there. There's an open discussion right now in #329 about moving the basic alignment data structures to biojava-core while keeping the substitution matrices in biojava-alignment.
From my side no other comments, all the other points sound like good ideas to me!
paolopavan commentedon Nov 12, 2015
Hello Jose,
If the phylo module does not depend on the alignment module and the current
situation in #329 see the substitution matrices in core, this would not
vote for leaving the substitution matrices stuff in core?
I mean that while conceptually I agree that a phylogenetic module may
depend on the alignment one, if the first does not use any of the
alignment routines in the second, a computer science common good practice
may suggest to not think in "biological meaning way", reduce the number of
dependencies and refer directly to core for substitution matrices.
For Aleix, the premise is that in a refactoring proposed for other things,
I have moved the substitution matrices into core. This wasn't a very much
intended action and I haven't been very happy with that but was necessary
because they are tight coupled with alignment data structures.
But if we intend the substitution matrices as a common data structure
because several independent modules may want to refer them, so in my
opinion it could make sense to leave them into core.
Moreover we could solve two problems at one.
Paolo
2015-11-11 2:37 GMT+01:00 Jose Manuel Duarte notifications@github.com:
lafita commentedon Nov 12, 2015
The situation right now is that the phylo module does not depend on the alignment module, but it contains its own DS for substitution matrices (called ScoreMatrix) and hard coded BLOSUM62 and PAM250 matrices. From my point of view, this is a duplication and should be solved by using the SubstitutionMatrix in alignment and the resources files that allow to obtain the different matrix values. If these are moved to the core module, the additional dependency of the phylo module will not be needed. Another strange dependency is that the alignment module depends on the phylo module, but nothing is used.
To me it seems a good idea to move the substitution matrices to the core module, as they are an important part of bioinformatics and that way we could keep phylo and alignment independent from each other. Introducing the alignment dependency into phylo, however, is not such a bad idea, because the user needs first a multiple alignment to generate the phylogenetic tree. The user could get the MSA by parsing a FASTA file (only needs core dependency), but it is probable that alignment features are needed at some point.
foreveremain commentedon Nov 12, 2015
FWIW you probably want to decouple substitution models from the alignment module. Although phylo and alignment require them, they are also needed for other kinds of downstream MSA analysis tasks.
I began to do exactly this with Jalview's architecture, since substitution based score models are not the only type of score models Jalview uses to build trees, and we also use score models for reordering sequences in the alignment.
keep up the good work folk !
josemduarte commentedon Nov 12, 2015
Alright there seems to be some consensus on keeping substitution matrices in core. We can do that and solve a few problems with it as @paolopavan says. That means I will go ahead and merge #329 as soon as I can and then we'll have both substitution matrices and alignment data structures in core.
@lafita regarding other strange dependencies, I definitely support removing the dependency of alignment on phylo. It doesn't make sense plus it's not used so no point in keeping it. If anything there should be a dependency of phylo on alignment (as you say likely to be needed in the future).
On a separate note, I found some old comments about the phylo module in #122 . It seems that the current phylo acts mainly as a wrapper to the forester package, see also the tutorial in the Biojava wiki. In any case I don't see any problems on having more phylogenetic stuff implemented within Biojava, so definitely go ahead with your plans @lafita !
josemduarte commentedon Nov 12, 2015
#329 is now merged
lafita commentedon Nov 13, 2015
Thanks @josemduarte!
The forester package is great in phylogeny reconstruction (and for the IO support it provides in the different formats, as well for the GUI to visualize trees), but I have seen that the distance calculation methods (evolutionary inference) are the very simple ones (it does not use substitution matrices, only basic models like Kimura or percentage of identity).
In the current state, biojava is already calculating the distances itself with some imported Jalview code using BLOSUM62 or PAM250 (hard coded), but the idea would be to generalize to use any substitution matrix the user wants (and using the SubstitutionMatrix DS of biojava) or any distance matrix. That way we could include other evolutionary models, like structure-based for example. The tree construction would be using forester.
As another point, forester does not provide support for Maximum Likelihood trees, but I was not intending to implement that.
lafita commentedon Nov 17, 2015
I realized that the alignment module uses the phylo module to build a GuideTree for progressive multiple sequence alignments. Actually, it uses directly the forester library, instead of using the wrapper functions that the phylo module provides.
I think the best is to keep the dependency of alignment on the phylo module to build the _GuideTree_s for MSAs, with the drawback that some scoring function will need to be defined outside the phylo module (the ones that use alignment functions).
josemduarte commentedon Nov 17, 2015
Another possible solution is simply merging the alignment and phylo modules. They are too inter-related to be in two different modules in my opinion: trees need MSAs and MSAs need trees. Joining them will avoid other dependency issues in the future.
Perhaps we could do that in next release now that we have already done some other major refactoring in alignment and core packages (see #329)
lafita commentedon Nov 17, 2015
I vote in favor of that option! In addition, the phylo module contains very few classes after cleaning it from unused Jalview code. Should I merge them in my working branch before doing a pull request?
sbliven commentedon Nov 20, 2015
+1 for merging
Merge alignment and phylo modules
andreasprlic commentedon Jan 12, 2016
-1 for merging, I'd prefer the alignment module not to have a dependency on forester.
lafita commentedon Jan 12, 2016
The alignment module already depends on the forester library, because it has a dependency on the phylo module. The reason is that a type of multiple sequence alignment, the progressive MSA, needs to construct a guide tree from all-to-all pairwise alignments, so it uses the TreeConstructor class (in phylo) to do so (which uses forester as the engine).
The main reason for merging both modules was that alignment needs phylo for the reason mentioned before, and phylo needs alignment methods to calculate pairwise evolutionary distances (based on sequence alignments).
sbliven commentedon Jan 26, 2016
I believe we decided that phylo and alignment should be merged, but not until after the 4.2 release.
lafita commentedon Feb 25, 2016
This issue can be closed, because the phylo module has been extended as described and the forester version updated so that everything is working.
I will create a new issue for 5.0.0 release with the alignment-phylo merge as topic.