Checking that SA trees use the right operators is problematic

[laduplessis]

Current state:

The tree class in beast2 core implements sampled ancestor trees, but the operators for adding/removing sampled ancestors in a tree are part of the sampled-ancestors package. By default a tree will be initalised without sampled ancestors and the regular tree operators will be valid.

Problem:

If a package implements a tree initialisation method that introduces sampled ancestors or implements a treeprior that assumes a sampled ancestor tree, it is left to the package designer to implement a test that sampled ancestor tree operators are being used instead of regular tree operators. This leads to code duplication since the same check needs to be implemented in multiple packages and scenarios.

Additionally, when multiple trees are being used there is a danger that the package will throw an error for operators on the wrong tree. Furthermore, it ignores the possibility of implementing new operators that either extend blacklisted operators, but can handle SA trees, or new operators that are not blacklisted but cannot handle SA trees.

Proposed solutions

Solution 1:

Add a boolean attribute hasSampledAncestors to the Tree class. In initAndValidate() the Tree class should then check that the correct sampled ancestors tree operators are being used instead of the regular tree operators. This places the check in a central location, but won't solve any issues with new operators.

Solution 2:

Add method canHandleSampledAncestors() to the TreeOperator abstract class (that returns false by default). Add a boolean attribute hasSampledAncestors to the Tree class. Then add initAndValidate() method to TreeOperator that throws an error if the tree has sampled ancestors, but the operator cannot handle them.

This will require adding super.initAndValidate() to all existing TreeOperators. (Since TreeOperator has no initAndValidate() method, most TreeOperators have a blank initAndValidate() method as well).

Solution 3:

Let SATree inherit from Tree. Operators, tree initialisers and tree priors now have to use the right type of tree. (I think this was actually the original behaviour with the original ZeroBranchSATrees wasn't it?)

[alexeid]

Proposal: Add a TreeSpace enum to Tree: public enum TreeSpace {BINARY_TIME_TREE, FOSSILIZED_TIME_TREE}; private TreeSpace treeSpace = TreeSpace.BINARY_TIME_TREE; public TreeSpace getTreeSpace() { return treeSpace; } This enum defines the possible tree spaces a tree could be constrained to, and the instance variable describes the tree space of this instance of Tree. Then add a method to TreeOperator that can test if an operator is compatible with a given tree, and give it a default that only compatible with binary time trees: public boolean compatible(Tree tree) { return tree.getTreeSpace() == Tree.TreeSpace.BINARY_TIME_TREE; } Then add a test in TreeOperator construction that calls compatible and fails on false. Then allow the user to specify the tree space in BEAUti.

…

On 8/09/2018, at 3:32 AM, Louis ***@***.***> wrote: Current state: The tree class in beast2 core implements sampled ancestor trees, but the operators for adding/removing sampled ancestors in a tree are part of the sampled-ancestors package. By default a tree will be initalised without sampled ancestors and the regular tree operators will be valid. Problem: If a package implements a tree initialisation method that introduces sampled ancestors or implements a treeprior that assumes a sampled ancestor tree, it is left to the package designer to implement a test that sampled ancestor tree operators are being used instead of regular tree operators. This leads to code duplication since the same check needs to be implemented in multiple packages and scenarios. Additionally, when multiple trees are being used there is a danger that the package will throw an error for operators on the wrong tree. Furthermore, it ignores the possibility of implementing new operators that either extend blacklisted operators, but can handle SA trees, or new operators that are not blacklisted but cannot handle SA trees. Proposed solutions Solution 1: Add a boolean attribute hasSampledAncestors to the Tree class. In initAndValidate() the Tree class should then check that the correct sampled ancestors tree operators are being used instead of the regular tree operators. This places the check in a central location, but won't solve any issues with new operators. Solution 2: Add method canHandleSampledAncestors() to the TreeOperator abstract class (that returns false by default). Add a boolean attribute hasSampledAncestors to the Tree class. Then add initAndValidate() method to TreeOperator that throws an error if the tree has sampled ancestors, but the operator cannot handle them. This will require adding super.initAndValidate() to all existing TreeOperators. (Since TreeOperator has no initAndValidate() method, most TreeOperators have a blank initAndValidate() method as well). Solution 3: Let SATree inherit from Tree. Operators, tree initialisers and tree priors now have to use the right type of tree. (I think this was actually the original behaviour with the original ZeroBranchSATrees wasn't it?) — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

[tgvaughan]

Great idea. Would it be possible to make the set of tree spaces extensible? Perhaps by using a TreeSpace class rather than an enum?

[alexeid]

Yes!

…

On 11/09/2018, at 8:05 PM, Tim Vaughan ***@***.***> wrote: Great idea. Would it be possible to make the set of tree spaces extensible? Perhaps by using a TreeSpace class rather than an enum? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#807 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA3WSaX_mXz14w0842Jlaxe-b0wfDZQpks5uZ264gaJpZM4WfCju>.

[rbouckaert]

Yes, adding a class to identify TreeSpaces makes sense.