vegandevs / vegan

R package for community ecologists: popular ordination methods, ecological null models & diversity analysis
https://vegandevs.github.io/vegan/
GNU General Public License v2.0
460 stars 98 forks source link

Regression in ordiplot(<rda-class>, scaling=1) #685

Closed jarioksa closed 1 day ago

jarioksa commented 2 months ago

The problem was found in reverse depedency tests for BiodiversityR examples:

> ### Name: PCAsignificance
> ### Title: PCA Significance
> ### Aliases: PCAsignificance ordiequilibriumcircle
> ### Keywords: multivariate
> 
> ### ** Examples
> 
> library(vegan)
> data(dune)
> Ordination.model1 <- rda(dune)
==== clip ====
> plot1 <- ordiplot(Ordination.model1, choices=c(1,2), scaling=1)
Error in match.call(definition, call, expand.dots, envir) : 
  formal argument "scaling" matched by multiple actual arguments
Calls: ordiplot ... plot -> plot.rda -> NextMethod -> plot.cca -> match.call
Execution halted

A short example is:

ordiplot(rda(dune), scaling = 1)
# Error in match.call(definition, call, expand.dots, envir) : 
#  formal argument "scaling" matched by multiple actual arguments

The bug does not occur in the release branch cran-2.6-6. It does not influence cca but it influences dbrda and capscale.

The problem was bisected to commit ff7566809 but fix may be needed in a file that was not touched in that commit.

jarioksa commented 2 months ago

The error really seems crop out on line 81 of plot.cca:

    dots <- match.call(expand.dots = FALSE)$...

which was added in ff75668 to handle dot arguments. However, this seems not work:

Seems to need re-thinking.

gavinsimpson commented 2 months ago

Shouldn't ordiplot have scaling, hill, correlation, and const? Right now it doesn't seem like you can change what scores get plotted except on which axes, and what types of scores, and ... is not being passed on to scores() in ordiplot(). (I appreciate that this is not really the source of the problem here, but still.)

jarioksa commented 2 months ago

I don't think so: scaling, hill, correlation and const only make sense with cca methods which are a marginal case for ordiplot and in most cases these arguments would be ignored. With "cca" and "decorana" classes we short-cut to plot.cca and plot.decorana and skip most of ordiplot, but for ordiplot proper and most ordination methods those arguments make sense.

gavinsimpson commented 2 months ago

Ignore the above I missed the second part of the conditional

if (inherits(ord, "decorana") || inherits(ord, "cca")) {
gavinsimpson commented 2 months ago

I have pushed an alternative fix to issue-685 branch as discussed in cafdc03 - I need to test this though - was there a specific test you were running to check this @jarioksa? I'm not 100% clear on how the new code all works for the piping behaviour. I think optimize worked where I passed it through ordiplot() on the example that failed from BiodiversityR, but it would be good to get a second opinion on that.

jarioksa commented 2 months ago

I used only ordiplot(rda(dune ~ Management, dune.env), scaling = 1) in git bisect (found later that rda(dune) is sufficient). I found the problems with optimize and arrows later and incidentally and didn't test or serch them. However, if you run R CMD check you should get tests for most cases with pipes and configurable plots.

jarioksa commented 2 months ago

I checked the use of NextMethod in vegan and it seems that everyone of them incorrectly uses .... They should be fixed.

$ grep -n NextMethod R/*R
R/add1.cca.R:12:    out <- suppressMessages(NextMethod("add1", object, test = "none", ...))
R/anova.prc.R:22:        NextMethod("anova", object, ...)
R/biplot.rda.R:12:        NextMethod("biplot", x, ...)
R/drop1.cca.R:11:    out <- suppressMessages(NextMethod("drop1", object, test = "none", ...))
R/goodness.metaMDS.R:5:        return(NextMethod("goodness", object, ...))
R/plot.cca.R:147:    NextMethod("plot", x, ...)
R/print.cca.R:118:    NextMethod("print", x, ...)
R/print.mso.R:4:    NextMethod("print", x, digits = digits, ...)
gavinsimpson commented 2 months ago

Some checking suggests that optimize passing from ordiplot() -> plot.cca() currently is not happening with NextMethod().

Rereading ?NextMethod suggests that my description in the commit comment is not quite correct and what you are doing should work, but the evidence is to the contrary and scaling etc are getting duplicated. I'll need to look into this further when I get back from the UK after the weekend.

jarioksa commented 2 months ago

The logics and arguments of ordiplot and plot.cca are non-concordant. If plot.rda is changed so that dots are removed in NextMethod("plot", x) then in the following command

ordiplot(rda(dune), scaling=2, type="t", bg="white", optimize=TRUE)

scaling, type and bg are passed to plot.cca, but optimize is not. However, in

ordiplot(rda(dune), scaling=2, type="t", bg="white", spe.par=list(optimize=TRUE))

optimize is passed to species in plot.cca. The arguments differ in the following ways:

Inspect the following command:

ordiplot(rda(dune), scaling=2, type="t", bg="white", spe.par=list(optimize=T), arrows = TRUE)

This will be passed to plot.cca as:

### with NextMethod("plot", x) or empty NextMethod() match.call()
plot.cca(x = ord, choices = choices, scaling = 2, type = type, 
    xlim = xlim, ylim = ylim, spe.par = ..3, ... = pairlist(bg = "white"))
### with NextMethod("plot", x, ...) sys.call()
### match.call() fails with multiple definitions, args after ... are dot args
plot.cca(ord, choices = choices, type = type, xlim = xlim, ylim = ylim, 
    ..., scaling = 2, bg = "white", spe.par = list(optimize = TRUE))
argument ordiplot plot.cca NextMethod no dots NextMethod w/dots
choices, type, xlim, ylim yes yes pass pass
scaling, spe.par no yes pass pass as dot arg with conflict
bg no no pass as a dot argument pass as a dot argument
arrows/optimize yes no do not pass do not pass

The minimum nuisance strategy is to add optimize and arrows among arguments of plot.cca and have NextMethod without dots

I focussed on plot.rda with NextMethod, but ordiplot(cca(dune), ...) works like NextMethod("plot", x). That is, it works OK but does not pass optimize and arrows which are not among formal arguments of plot.cca.

jarioksa commented 1 week ago

@gavinsimpson do you want to finish this, or should I have a look at the issue? This breaks BiodiversityR, and should be fixed.

gavinsimpson commented 4 days ago

@jarioksa realistically, I'm unlikely to have time to focus on this until I am finished teaching for this semester (last day is Dec 6). I can take a look then, but feel free to tackle this in the meantime.