Skip to content

Fix submodel indexing logic in ModelMarkov functions, fix likelihood calculations in AlignmentPairwise - #194

Open
StefanFlaumberg wants to merge 3 commits into
iqtree:masterfrom
StefanFlaumberg:fixmodel
Open

Fix submodel indexing logic in ModelMarkov functions, fix likelihood calculations in AlignmentPairwise#194
StefanFlaumberg wants to merge 3 commits into
iqtree:masterfrom
StefanFlaumberg:fixmodel

Conversation

@StefanFlaumberg

Copy link
Copy Markdown
Contributor

This pull request introduces the following changes:

In ModelSubst:

  • Descriptions and signatures are changed a bit for several functions, see more on this in the next section.
  • Cosmetic refactoring with no algorithmic changes except for 1. setting a nullptr in the destructor and 2. using 1.0 / (num_states-1) instead of 1.0/3 in getQMatrix().
  • Transfer the implementation of the multiplyWithInvEigenvector() function to ModelMarkov to prevent accessing nullptrs.
  • Note: In fact, ModelSubst is a half-dead-code class. It is not used directly anywhere, but only as a base-class pointer for ModelMarkov. Since the JC model is a Markov model itself, making this class the base class of ModelMarkov breaks the common sense in the inheritance logic. This class should be merged into the ModelMarkov class in the future, maybe, as a special case representing the default behaviour.

In ModelMarkov and its derived classes:

  • Rename setFullRateMatrix() to setQMatrix() for clarity and call setStateFrequency() in it independently of the model reversibility.
  • Add the non-reversible case in the derivative-computing computeTrans() overload.
  • Remove the computeTrans() overloads that lacked the model_id argument and add this argument to the both ModelFactory::computeTrans() functions, which work as wrappers for the remaining computeTrans() overloads.
  • The important part: Use robust logic for accessing submodels in various ModelMarkov functions (fixes the problem discussed in Bug fix for PMSF+I combinations #168 (comment)):
    -- Both remaining computeTrans() overloads, computeTransMatrix(), and computeTransDerv() now have the model_id argument defaulted to -1. In simple models the argument is unused. In ModelMixture and ModelSet, the argument selects the respective submodel to redirect the function call to it, the default argument explicitly leads to a failed assertion. Conversely, the upstream version used 0 as the default, thus permitting calls without the argument that implicitly used the front model -- a behaviour clearly unwanted.
    -- getStateFrequency(), getRateMatrix(), getQMatrix() now all have the model_id argument defaulted to -1. In simple models the argument is unused. In ModelMixture and ModelSet, non-default values call the function for the respective submodels, whereas the default value leads to a failed assertion for getRateMatrix() and getQMatrix(). The default value results in the mean class frequencies in ModelMixture and in the +F frequencies in ModelSet for getStateFrequency() as it did before. The rationale is that for ModelMixture and ModelSet there exist no notion of the default rate matrix in the general case.
    -- setStateFrequency(), setRateMatrix(), setQMatrix() don't have the the model_id argument and (except for adaptStateFrequency() in ModelMixture) fail when called for mixture models or ModelSet. These functions should be directly called for the submodels by classes that have access to these submodels, so the rationale is encapsulation.
    WARNING: the PhyloTree::computeMarginalAncestralState() function is left not adapted for ModelSet (incorrect call of getStateFrequency()) because it uses the nonrev kernel anyway; similarly, PhyloTree::computeAncestralLikelihood() remains incompatible with ModelMixture and ModelSet (incorrect call of getStateFrequency() and computeTransMatrix()) because the function is an unused code. This should probably be solved later.

In AlignmentPairwise:

  • Merge the isRateCategorized and the "usual rates" cases in setSequenceNumbers(). Fix setting total_size in the constructor.
  • Merge the implementations of computeFunction() and computeFuncDerv() into a private template function. Remove the sum_trans_mat member buffer, as it was playing the same role as the sum_trans buffer.
  • Properly compute the pairwise likelihood and its derivatives for all possible combinations of simple/mixture/site-specific models and rates. Of special practical importance, 1. mixture models are now accounted for in the "usual model" section, 2. rate mixtures and +I are accounted for in the "site-specific model" section, which fixes the problem mentioned earlier in PMSR model implementation iqtree2#391 (comment).
    WARNING: This may affect the distance matrices and trees calculated under such commonly used models as LG+Cxx+G4 and LG+SSF[Cxx]+G4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant