emmanuelparadis / ape

analysis of phylogenetics and evolution
http://ape-package.ird.fr/
GNU General Public License v2.0
52 stars 11 forks source link

Added a multiPhylo case to makeNodeLabel #102

Closed TGuillerme closed 9 months ago

TGuillerme commented 10 months ago

Dear ape team,

Here's a quick and easy self call for makeNodeLabel to handle the phy argument as a "multiPhylo" with a mini test below if it's of interest (feel free to reject if not interested!).

Cheers, Thomas

test_that("makeNodeLabel works for phylo and multiPhylo", {
    set.seed(1)
    trees <- rmtree(2, 3)

    ## phylo
    test <- makeNodeLabel(trees[[1]], prefix = "Node")
    expect_is(test, "phylo")
    expect_equal(test$node.label, c("Node1", "Node2"))

    ## multiPhylo (with options parsed correctly)
    test <- makeNodeLabel(trees, prefix = "nOde")
    expect_is(test, "multiPhylo")
   expect_equal(test[[2]]$node.label, c("nOde1", "nOde2"))
})
emmanuelparadis commented 9 months ago

Hi Thomas,

What you suggest is actually to make makeNodeLabel() a generic function: this is a great idea!

I suggest to do this explicitly, so methods for other classes could be added later (if needed in their time). This would imply that your proposed addition would be a separate function (i.e., makeNodeLabel.multiPhylo()) and the current makeNodeLabel() would become makeNodeLabel.phylo().

For the tests, there is a (relatively) new package apeTests that compiles them (they are too heavy to run on CRAN).

What do you think? Cheers, Emmanuel

TGuillerme commented 9 months ago

Hi Emmanuel,

Yes good idea! I'll work on that when I'll have time and update this pull request accordingly.

Cheers, Thomas

emmanuelparadis commented 9 months ago

Yes good idea! I'll work on that when I'll have time and update this pull request accordingly.

Don't worry for this. I'll take your code and adjust the other files (several files need to be modified, inc. NAMESPACE, which I can do easily from here). Cheers, E.

TGuillerme commented 9 months ago

Nice thanks! I think one time it could be good to generalise this .phylo .multiPhylo class handling. There's nothing urgent or very important but it seems easy to lapply most standard .phylo functions in the same way as done for this example (either through self referencing or properly through declaring the two class functions).

emmanuelparadis commented 9 months ago

Done. The test has been added to apeTests.