Fix consensusTree()

[davidcerny]

This branch reverts one of the readability changes introduced by @bredelings into the TreeSummary::mrTree() function in commit 2114c4b. As it turns out, this was the source of issue #361. I have verified that after changing the iterator back to what it was, RevBayes produces the same output as in v1.1.0, i.e., the version which was reported to still work correctly by the author of the issue.

Note that I haven't touched any of the other function in TreeSummary, many of which had analogous changes made to them. I haven't checked if any of those suffer from similar issues.

[bredelings]

Oh drat. So the iterator version is walking the list in reverse, but my "readable" version is not.

It looks like there are 2 other instances where I switched a reverse walk to a forward walk. (i.e. they used .rbegin() and .rend())

[davidcerny]

Ah, so that was it! Feel free to add commits to this branch as needed, of course

[bredelings]

Hmm... I'd like to fix the other 2 cases in this file, and I also think we should add the test case from #361. Instead of just reverting the readability change, I have a way of fixing it.

[bredelings]

OK, so I have fixed the bug in a different way, and also addressed the readability of for-loops doing a reverse walk.

Basically, anything that was iterating over a std::set<Sample<T>> and then doing a break to quit the loop when the count gets low enough needs to iterate in a reverse fashion, because the highest-probability items are at the end of the list.

When this finishes compiling on my slow laptop, I'm going to check that the we now get the right consensus tree for #361. Would it be possible for you to make a test-case out of this?

I'm thinking we need to add a new test_consensus directory. The new test script should start with seed(12345) and end with q() I think (just based on what the others do). It should load the trees, and then write out the majority consensus to output/, and then we should put the real output in output_expected. It would be nice to also generate (say) the mcc to a different file.

Does that sound doable for you, or should I do it?

[davidcerny]

That sounds doable; I've written tests before. I'll get right on it

[bredelings]

Fixes #361

[bredelings]

Nice test case.
Thanks for finding this!

[davidcerny]

Thank you for solving the issue in a much more elegant way!