Exchange operator contains code that is never executed

[tgvaughan]

The narrow() method of the Exchange class contains an "alternative" implementation of the proposal that is wrapped in the else block following an if (true). No-doubt this is optimized out, but surely we can get rid of this? It's been unused for 4 years, apparently.

[jheled]

Actually the "alternative" code was useful at least once, when Sasha noticed a bug which disappeared when the alternative was used instead. (I fixed the code, obviously) .

-Joseph

[tgvaughan]

I'm sure it has been useful. Actually, it's the code marked as "alternative" that's been used for the past 4 years, isn't it? If someone wants to use the stuff in the else block, perhaps we need to wrap it in a different operator - or let users select it via a boolean input or something. I remember running into this a few years ago, but figured I'd do something about it this time. ;-)

[jheled]

The original code (which is under the 'false' branch) was taken from BEAST1. I did not like the fact that to get the right Hastings ratio it "failed" about half the time, and figured another way to sample the nodes that will not need to do that. But I make an error with the Hastings ratio initially, which I fixed later/

That is just the history and a (very weak) explanation why I sometimes keep old code around.

[tgvaughan]

I definitely like your method better. Similar things are done in the WB operator. As for keeping old code: one of the nice things about version control is that the old code is kept safely for us. ;-)

[alexeid]

So narrow() in BEAST2 is a different operator from BEAST1 code? Shouldn’t we wrap the new operator in a different class, rather than change the behaviour of a published operator?

On 30/03/2016, at 9:53 am, Tim Vaughan notifications@github.com wrote:

I definitely like your method better. Similar things are done in the WB operator. As for keeping old code: one of the nice things about version control is that the old code is kept safely for us. ;-)

— You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub https://github.com/CompEvol/beast2/issues/540#issuecomment-203097468

[jheled]

It is the "same" operator in the sense it does the same operation on the tree. The difference is that the "new" version is never rejected at the operator level, while the BEAST1 version does. I would call it the same operator, but obviously you can have both, only then you need to decide which to use in BUEAti and how to provide the other form.

[alexeid]

Rejecting invalid proposals is the simple way to let MCMC do the counting for you. If you do the counting yourself accurately then great, but it is more likely to have a bug, or be broken later unless you have good tests. Like you say: having foolproof basic operators can be handy to have around when you design fancy operators that are more efficient.

Cheers Alexei

On 2/05/2016, at 4:23 pm, Joseph Heled notifications@github.com wrote:

It is the "same" operator in the sense it does the same operation on the tree. The difference is that the "new" version is never rejected at the operator level, while the BEAST1 version does. I would call it the same operator, but obviously you can have both, only then you need to decide which to use in BUEAti and how to provide the other form.

— You are receiving this because you commented. Reply to this email directly or view it on GitHub https://github.com/CompEvol/beast2/issues/540#issuecomment-216103014

[jheled]

We can add a flag to the exchange operator to select between the two options. But then you need to make a choice RE the default version to use in BEAUti. I have zero objections to making it the battle-tested-but-less-efficient BEAST1 version since I typically generate my own XMLs.

[alexeid]

The nice thing about sub classing and object orientation is that classes that are well defined can be simply implemented. Having all the options in a single class makes things more prone to misunderstanding and error. For a simple implementation to be simple it should be cleanly separated from the complexity right?

Sent from my iPhone

On 2/05/2016, at 4:36 PM, Joseph Heled notifications@github.com wrote:

We can add a flag to the exchange operator to select between the two options. But then you need to make a choice RE the default version to use in BEAUti. I have zero objections to making it the battle-tested-but-less-efficient BEAST1 version since I typically generate my own XMLs.

— You are receiving this because you commented. Reply to this email directly or view it on GitHub

[jheled]

Implementing the "new" version in a derived class would involve code duplication. It was not written with extension in mind (my personal opinion, I may be wrong). Not a lot of code duplication, obviously, as this is a simple class, but the user would see a different operator name, which might be confusing.

Again, I am not against any change that would improve BEAST2. I like the fact that BEAST2 can improve over BEAST1, but if the price is deemed too high I have no objection to changes. We can easily move the new version to BEASTLabs and return the old code as the default to everyone else.

[alexeid]

I like your change Joseph. Please keep it. You know better than me what is the right thing to do.

On 2/05/2016, at 6:20 PM, Joseph Heled notifications@github.com wrote:

Implementing the "new" version in a derived class would involve code duplication. It was not written with extension in mind (my personal opinion, I may be wrong). Not a lot of code duplication, obviously, as this is a simple class, but the user would see a different operator name, which might be confusing.

Again, I am not against any change that would improve BEAST2. I like the fact that BEAST2 can improve over BEAST1, but if the price is deemed too high I have no objection to changes. We can easily move the new version to BEASTLabs and return the old code as the default to everyone else.

— You are receiving this because you commented. Reply to this email directly or view it on GitHub

[tgvaughan]

While not as battle tested as the BEAST 1 version perhaps, keep in mind that the Joseph's "new" version has been in every version of BEAST 2 for the last four years, and has been implicitly tested very carefully by anyone whose done proper validation of MCMC algorithms that use the standard tree operator set during this time.