search

beast-dev / beast-mcmc

Bayesian Evolutionary Analysis Sampling Trees
http://beast.community
GNU Lesser General Public License v2.1
192 stars 73 forks source link

Fix tests for OU gradients, `BigFastTreeIntervals` error #1203

Closed pbastide closed 2 months ago

pbastide commented 2 months ago

This patch fixes the following previously failing tests:

These 4 tests now pass with reasonable tolerance (for gradients), and the tested statistics match with the R computed ones. I'm not sure what caused these tests to break in the first place, as I could not pinpoint the exact moment they started failing. Has anything changed on random number generation ?

The CI still fails further down the road, during the junit testing (which did not run on previous builds, because of early failure in the TestXML files). It looks like it is linked with BigFastTreeIntervals, in particular on this line of test. I'm not sure how to fix this.

msuchard commented 2 months ago

@pbastide -- i don't feel it's a good idea to increase the tolerance to pass a test. i feel we need to explore what code-change caused these tests to start failing in the first place.

msuchard commented 2 months ago

as far as i can tell (from @xji3), these tests started to fail in the master branch (so that's a good place for a bisection search) and are possibly related to some tree-interval changes. that, however, does not seem to be the whole issue, as many of the failing tests don't use tree-intervals. i am guess that (at least) two separate changes got made.

pbastide commented 2 months ago

Yes, I agree that a bisection search would be better. Maybe the junit failing test in BigFastTreeIntervals here could be a first place to look ? However I agree I don't see the direct link between tree-intervals and gradient tests.

pbastide commented 2 months ago

After a search on master, it looks like commit 492022c is causing testLoadingsGradient.xml and ci/TestXML/testJointPartialsProvider.xml to fail first (these two tests still pass locally on my machine at d687ef1). Not sure how these are linked ?

pbastide commented 2 months ago

The two other tests ci/TestXML/testRepeatedMeasuresOUHmc.xml and ci/TestXML/testRepeatedMeasuresOUHmcTreeScaled.xml start failing earlier, at 6df6031. It also seems to be linked with the power optimization schedule. In that case, if the change in schedule changed the MCMC chain moves, then this could explain why the final value of the statistic changed. Maybe a test that does not rely on the final state of the chain would be more robust ? (My bad, I can change this.)

xji3 commented 2 months ago

@pbastide I think you found the commit, but it was not POWERB, but actually POWERC causing the trouble. It was changed from 0.55 to 0.5556. This commit b8c5f3d4a892c71a471cbacd32828d5e655789c8 seems to have recovered the old behavior and fixed the tests on hmc-clock.

The other test that Marc mentioned was the BigFastTreeTreeIntervalTest. And I think that one was caused by newly adopted FastIntervals created at line 111 - 112 in TreeIntervals https://github.com/beast-dev/beast-mcmc/blob/5ab4c5ecaf0186544d1179119c93c84f1441cedb/src/dr/evomodel/coalescent/TreeIntervals.java#L111-L112 not returning 0 (i.e., most recent tip node time) for getIntervalTime(0)... Maybe @rambaut here to correct me...

After a search on master, it looks like commit 492022c is causing testLoadingsGradient.xml and ci/TestXML/testJointPartialsProvider.xml to fail first (these two tests still pass locally on my machine at d687ef1). Not sure how these are linked ?

pbastide commented 2 months ago

Thank you for your feedback. Sorry, I should have checked hmc-clock before opening this PR. I will close it now, as the proposed commit is obsolete compared to b8c5f3d as you mentioned.