Fix `[<-.multiPhylo`

[ms609]

This looks like a more robust implementation of [<-.multiPhylo which should avoid the downstream issues with TreeTools et al. I've added some test cases to catch some of the odd behaviour; more test cases may be desirable?

[ms609]

Hi Emmanuel, is this likely to be included in the release to CRAN next week?

[emmanuelparadis]

Hi Martin, I'm checking your proposed fix for [<-.multiPhylo: it seems it would do the opposite of what the operator is doing now. Is that correct?

[ms609]

The intention is that the behaviour remains unchanged when a parameter is specified, e.g. trees[1] <- ...

The fix allows no parameter to be specified, when tip labels are specified using a TipLabel attribute, i.e.

trees[] <- someFunc(trees)

The new tests hopefully help to clarify the intent.

Written in haste – let me know if I need to clarify later!

[emmanuelparadis]

In fact, you meant to fix this bug:

R> TR
10 phylogenetic trees
R> TR[] <- TR
R> TR <- .compressTipLabel(TR)
R> TR[] <- TR
Erreur dans `[<-.multiPhylo`(`*tmp*`, , value = list(list(edge = c(11L, 12L,  : 
  l'argument "..1" est manquant, avec aucune valeur par défaut

? I'm checking several functions now. I think it was a mistake to use the ... in these operators since these lists are always single-dimensional. I'll have a closer look and will push a version in the next days that will also fix the above.

[ms609]

Yes, that's a simple way to reproduce it

I'm not sure that this is an ideal example, as .compressTipLabel(TR) removes the $tip.label entry from each tree in TR. As such, if TR does not already contain a TipLabel attribute, all tip information is removed by the []<- assignment.

What I was less confident about is what the correct behaviour should be regarding whether the "TipLabel" parameter is retained: as there is no guarantee that the new contents of TR[] <- UnknownFunction(TR) has the same tip labels as TR, my sense was that the safest thing to do was to drop the TipLabel attribute from the returned output.

[emmanuelparadis]

I pushed a new version here that should (may?) solve this.

[ms609]

Thanks, I've pulled the new code through, commenting out mine in this PR.

When I run through the tests in tests/testthat/test-summary.phylo.R I see:

  set.seed(0)
  twoTrees <- c(rtree(6), rtree(6))
  trees <- twoTrees
  trees[] <- trees

Error in x[..1] <- value : 
  ..1 used in an incorrect context, no ... to look in

[emmanuelparadis]

Your example works for me with ape 5.6-2 (dated as of today). Maybe you need to update your branch. There's no more use of ..1 in ape.

[ms609]

You're right, I've switched to the master branch, and this error has disappeared. Not sure why that didn't pull through first time.

However, at line 41 in the test file, trees[] <- lapply(trees, Capitalize), I see

Error in `[<-.multiPhylo`(`*tmp*`, i, value = 0L) : 
  at least one element in 'value' is not of class "phylo".

Yet lapply(lapply(trees, Capitalize), class) shows all elements as having class phylo.

[emmanuelparadis]

The calls to .uncompressTipLabel implied to restore the class to "multiPhylo" inducing a recursive call of the operator. Should be OK now.

[ms609]

Looks great, thanks for all your work on this! I've updated this PR with the code from the master branch, so merging this PR will add the test cases to ape to catch any future regressions.