BEAST2-Dev / bdsky

Birth Death Serial Skyline Model for BEAST2
GNU General Public License v3.0
2 stars 11 forks source link

BDSKY has been failing a unit test since January 2019. #28

Closed tgvaughan closed 4 years ago

tgvaughan commented 4 years ago

One of the likelihood SABirthDeathSkylineModel unit tests testLikelihoodCalculationTwoIntervalsWithRhoConditionOnRhoSamplingAndRoot was broken by a merge commit 6f6e6d489dd5415e316dea51287233e3996c4ae7.

One of the two lineages involved in the merge introduced this test, together with an R script for computing the likelihood value. This lineage was released as v1.4.3. The merge with the second lineage resulted in the modification of the following snippet in BirthDeathSkylineModel:

 rho[i]= //rhoSamplingChangeTimes.contains(times[i]) ? rhos[rhoSamplingChangeTimes.indexOf(times[i])] : 0.;
    (rhoChanges>0 || rhoSamplingTimes.get()!=null && rhos.length>1)?
      rhoSamplingChangeTimes.contains(times[i]) ? rhos[rhoSamplingChangeTimes.indexOf(times[i])] : 0.
      : rhos[0]
      ;

with the addition of the && rhos.length>1 expression. Removing this expression allows the unit test to again pass.

Once we fix this, I suggest adding automatic unit testing to this repository to help prevent this kind of thing happening again. BDSKY is a very popular BEAST 2 package, and although bugs happen, I think we need to try harder to ensure preventable errors like this don't appear.

tgvaughan commented 4 years ago

In 18624e6 I've reverted the commit referenced in the issue above, as it was definitely causing the test likelihood evaluation to produce incorrect results. (The test in question involves two intervals and one rho sampling event, and the && rhos.length>1 was causing BirthDeathSkylineModel to interpret the model as involving two rho sampling events: one at the present, and one at the interval boundary.

It's highly likely that this reversion will break whatever it was designed to fix, but as there are no unit tests for this I can't be sure. Maybe @denisekuehnert can look into this? :-)

tgvaughan commented 4 years ago

I've also revised and replaced the automatic unit testing (using travis-ci.org), which will hopefully prevent this and other regressions from happening in future.