Fix some issues with sampled ancestors

[bredelings]

In an attempt to get back to a "known good" state, this PR reverts a change to setAge( ) that I made last year. It also enforces that only tip nodes can ever have the sampled_ancestor flag set.

But it turns out that RootTimeSlideUniformProposal( ) has been moving fossils at the root since it was introduced in April 2023. This is unrelated to the changes that I had made. So now this PR modifies RootTimeSlideUniformProposal( ) to avoid changing the age of the root node if it is a sampled ancestor. (NodeTimeSlideUniformProposal() already does this.)

The PR also fixes some crashes in the MCC and MAP trees with sampled ancestors, where we need to represent the topology as a newick string with true sampled ancestors (i.e. knuckles) but then convert back to sampled-ancestors-as-tips afterwards. At least that is what the current code expects.

The PR also fixes the problem that mvNodeTimeSlideUniform would hang forever if there is not a moveable node on the tree. It does this by adding an exception type that cancels the proposal. This probably needs some review...

The PR also adds two test cases.

• a test case based on the bears example from [BUG] FBD model with sampled ancestors #401
• a test case with 2 extant taxa and 1 fossil taxa where the fossil taxon spends about 60% of the time as a sampled ancestor.

Finally, the PR speeds up running the windows tests by removing the old behavior of copying rb.exe for every test, which is no longer needed.

For some reason there are different test results only on windows for 1 new test case, and only on Mac/ARM for the other test case. My (temporary) solution is just not the check the results in those two cases.

[bjoelle]

I don't have any strong objections to these changes, but I also don't think they represent a fix for the issue of SAs moving since changes of SA ages are still happily allowed
see my suggestion in detailed comments

[bjoelle]

I don't understand what the propagate argument is used for here
also I don't think we should silently change the age of SA, considering this is what triggered the whole issue ? my proposal would be to have an argument allow_SA_changes that is by default false - if argument is true, proceed otherwise throw an exception
then we can set it to true only for things that are explicitly allowed to change taxon ages (i.e. only TipTimeSlideProposal at the moment)
bonus - it will tell us immediately which things are trying to change SA ages without being allowed to

[bredelings]

This is a revert to the previous state to make sure that I didn't break it. Now that it is clear that I didn't break this, I think we can go ahead with a real fix, although I guess we still have to make sure not to break anything in the process.

Sorry, when I first messaged you I thought that this changed fixed the problem, but then I realized that it did not.

The propagate argument seems to mean that when modifying the age of a sampled ancestor tip, one should change the parent as well.

One option is to modify setAge so that changing the age of the sampled-ancestor-parent always aborts. The rule would be that if you want to modify the and of a sampled-ancestor, you have to always modify the age of the tip node.

But I kind of like your idea of allowing changes via the sampled-ancestor-parent if allow_SA_changes is true.

Another idea was to make a new routine called proposeAge that is a wrapper around setAge that is used in MCMC moves and proposals. Then we can put the restrictions on proposeAge instead of modifying setAge directly, which would complicate lots of non-proposal operations.

[bredelings]

Also: why is it so important that only mvTipTimeSlideUniform changes the node ages? Is there an assumption that dnFBDP sets the initial age of fossils to their minimum age, so that you can clamp the minimum age by just not using any moves that change tip times?

It seems logically wrong to use the MCMC moves to specify the model. If the ages shouldn't be changed, then ideally they would be clamped, but if not clamped then at last the max age should be set to the min age.

[bjoelle]

so in practice I think analyses with fossil samples which have a fixed age or a narrow range of ages could become very inefficient if moves are allowed to change node ages without caring about the type of node, but then rejected when/if it doesn't work with a SA
I am also trying to prevent unintended broken setups - right now it's very clear from a user perspective whether they intend to estimate tip ages (in which case they include the mvTipTimeSlideUniform) or not, and I think it would be good to keep it that way
and it's also a good early warning for someone coding a new move who hasn't thought about that issue and maybe should
of course it could be that I'm just trying to replicate what I'm used to in BEAST2, so take that with a grain of salt :)

[bredelings]

• mvTipTimeSlideUniform already looks at the age constraints for a node in order to avoid inefficiency. We could replicate that in mvNodeTimeSlideUniform. In theory we could even combine the two moves.
• I don't want to break user setups, and I think it should be clear whether tip ages are fixed. But I don't think it should be based on whether or not an MCMC move is included.
• Can we specify whether tip ages are fixed by how we read taxon data? We could split readTaxonData( ) into readTaxonAges( ) and readTaxonAgeRanges( ), for example.
• I used to like the early warning idea, but now I feel more conflicted about it.
• I definitely do not want to replicate BEAST2 without a good reason :-P

[bjoelle]

• I was more thinking of tree moves such as tree scaling, tree rearrangements etc which may need special handling for SAs nodes
• my objection to splitting readTaxonData is that it's not going to be backwards-compatible with previous analyses, and we have already enough issues open due to function name changes :p

[bredelings]

OK, I modified mvRootTimeSlideUniform so that it doesn't move fossils anymore.

I also added member procedures .setMinAge( ) and .setMaxAge( ) to taxon objects.

I think a full solution to this issue would specify that taxon ages are fixed in the model and not the MCMC moves.

I haven't yet implemented a partial solution that complains when people call setAge on a sampled ancestor node.

However, I'd be happy to get this merged as-is (if that's OK), and leave other fixes to a followup PR. What do you think?

[bjoelle]

ok for merging as-is and we can discuss other changes later