Not using SA operators for SA analysis should be a fatal error

[laduplessis]

Current behaviour when running a sampled ancestors analysis without using sampled ancestor tree operators is to print an error message stating the analysis is not valid and continue running. The error message is printed in between other initialisation output and will probably not be noticed by many users. If the analysis is not valid the error should be fatal and the program should refuse to run.

[tgvaughan]

This is actually a bit dangerous, as the test method only checks whether any members of an operator blacklist are present in the analysis. This ignores the possibility that there are multiple trees in the analysis, only some of which are SA trees, and would prevent these more complicated analyses from running.

Furthermore, it's possible that someone might design an operator which extends one of these blacklisted operators but is compatible with SA trees. This check would prevent use of such an operator.

Also, nothing's to stop someone from using an operator not on this list that is similarly incompatible with SA trees. In that case the check would pass, but the analysis would actually be invalid.

I think a better approach here would be to 1. tighten up the conditions that cause the warning to appear, and to 2. make this warning harder to ignore, for instance by prompting the user to decide whether to continue.

Sadly none of this would be a problem if SATrees were a class/interface of their own, or we had an interface like BinaryTree: the operators could then specify exactly what kind of tree they are applicable to.

[laduplessis]

I agree. This is a little tricky, because it puts the responsibility of checking operator validity on the package designer. I think this is actually an issue that should be fixed in the core, since sampled ancestor trees are now supported in the core. I'm opening up an issue in the beast2 core about this. Anyway, just flagging the issue, I don't think it has a very high priority.

[tgvaughan]

Thanks Louis, just read through your issue - looks great!

[gavryushkina]

Since we have SA trees in the core we have to assume that it is a very standard type of trees and SA operators should be in the core too. Or core operators should be changed to handle SA trees as well. Perhaps using a flag whether to run an SA analysis or a binary analysis. Then, if someone develops a new operator they should again take into account that BEAST assumes SA trees are standard and either implement the behaviour for both types of trees or use a flag for binary analysis only. Otherwise I agree SA should not be in the core.

On Sat, 8 Sep 2018 at 03:43 Tim Vaughan ***@***.***> wrote: Thanks Louis, just read through your issue - looks great! — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#13 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGJfcinxuy_S9c761-xK5MT0svL_8d1pks5uYpQhgaJpZM4We6Ym> .

-- Best regards, Dr. Alexandra "Sasha" Gavryushkina Postdoctoral fellow Department of Biochemisty University of Otago Dunedin, New Zealand

[laduplessis]

Here is the issue I posted on the beast2 core repo: CompEvol/beast2#807

[tgvaughan]

I definitely agree with Sasha that the SA operators should be in the core.