-
Notifications
You must be signed in to change notification settings - Fork 99
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conflict with rda object (Regularized Discriminant Analysis) in klaR #277
Comments
diff --git a/R/plot.cca.R b/R/plot.cca.R
index 7c7130f..cd17643 100644
--- a/R/plot.cca.R
+++ b/R/plot.cca.R
@@ -132,3 +132,6 @@
class(g) <- "ordiplot"
invisible(g)
}
+
+`plot.rda` <-
+ plot.cca
diff --git a/R/print.cca.R b/R/print.cca.R
index 685ccca..afd6655 100644
--- a/R/print.cca.R
+++ b/R/print.cca.R
@@ -70,3 +70,5 @@
}
invisible(x)
}
+
+`print.rda` <- print.cca
diff --git a/NAMESPACE b/NAMESPACE
index 35683ef..1efaf77 100644
--- a/NAMESPACE
+++ b/NAMESPACE
@@ -319,6 +319,7 @@ S3method(plot, rad)
S3method(plot, radfit)
S3method(plot, radfit.frame)
S3method(plot, radline)
+S3method(plot, rda)
S3method(plot, renyi)
S3method(plot, renyiaccum)
S3method(plot, spantree)
@@ -389,6 +390,7 @@ S3method(print, protest)
S3method(print, radfit)
S3method(print, radfit.frame)
S3method(print, radline)
+S3method(print, rda)
S3method(print, specaccum)
S3method(print, simmat)
S3method(print, simper) |
Actually, vegan seems to work as documented. Including correctly dispatching the unmodified plotting data in |
issue #94 seems to concern the same name conflict. |
It seems that klaR defines the following S3 methods for its This is a similar issue as we had with All we can do is to make vegan functions aware of klaR functions and issue a warning. It seems that |
In my particular case, I'm not interested in actually using |
Added vegan:::print.rda that does nothing but delegates printing to NextMethod (i.e., vegan:::print.cca). Otherwise klaR:::print.rda may be used with obscure results. There is a similar name conflict with klaR:::plot.rda because vegan has no plot.rda function but delegates all plotting to vegan:::plot.cca. Addresses github issue #277.
I think I have now solved the issues as well as they can be solved. There is a genuine name conflict and a completely satisfactory solution cannot be reached. The changes are now in issue-#277 branch ready to be merged. The changes are:
If nothing strange appears, this will be merged within 24 hours. |
The situation seems to be more complicated than I assumed. The changes I made solved the problem in my macOS laptop with R 3.4.4 when vegan was loaded after klaR and A simple test procedure is: library(klaR)
library(vegan) # load after klaR
data(dune, dune.env)
rda(dune) # print worked in macOS, failed in Linux
rda(dune ~ ., dune.env) # print worked in macOS, failed in Linux Here failling means that the function used |
It works correctly for me in Linux, R 3.3.2. |
Yes indeed. The situation is still more complicated than I assumed: the working depends on the directory used when launching the command, and when started as However, this means that the changes may mitigate the problem at least occasionally. I'll ponder that for a while before deciding what to do with the merge. |
It appears that the change introduces a problem in library(vegan)
data(dune, dune.env)
r <- rda(dune ~ ., dune.env)
ordiplot(r, display = "species")
#Error in list(1, slam, sqrt(slam))[[abs(scaling)]] :
# subscript out of bounds
#In addition: Warning message:
#In if (scaling) { :
# the condition has length > 1 and only the first element will be used It looks like this is because This seems to fix it for me: diff --git a/R/ordiplot.R b/R/ordiplot.R
index f3dda10..36e663b 100644
--- a/R/ordiplot.R
+++ b/R/ordiplot.R
@@ -10,9 +10,9 @@
if (!is.null(attr(ord, "class")) && (class(ord) == "decorana" ||
any(class(ord) == "cca"))) {
if (missing(display))
- out <- plot(ord, choices, type = type, xlim = xlim,
+ out <- plot(ord, choices = choices, type = type, xlim = xlim,
ylim = ylim, ...)
- else out <- plot(ord, choices, type = type, display = display,
+ else out <- plot(ord, choices = choices, type = type, display = display,
xlim = xlim, ylim = ylim, ...)
}
else { |
I think I have figured out what was going on with my failure to fix To reproduce this, start with empty Worskpace. Mere sessionInfo()
## gives among other items:
# loaded via a namespace (and not attached):
# [1] compiler_3.6.0
library(klaR)
library(vegan) # load after klaR
data(dune)
rda(dune) ## printed with vegan::print.rda
## Add an ecological nullmodel
nm <- nullmodel(dune, "curveball")
## quit saving the workspace
q("yes") Now start a new session that loads the previously saved Workspace ( sessionInfo()
## gives among other items:
# loaded via a namespace (and not attached):
# [1] compiler_3.6.0 MASS_7.3-49 Matrix_1.2-14 parallel_3.6.0
# [5] tools_3.6.0 mgcv_1.8-23 nlme_3.1-137 grid_3.6.0
# [9] permute_0.9-4 vegan_2.6-0 cluster_2.0.7-1 lattice_0.20-35
library(klaR)
library(vegan) # load after klaR
data(dune)
rda(dune) ## printed with klaR::print.rda
#Call:
#rda(X = dune)
#
#Regularization parameters:
#[1] "this is a vegan::rda result object"
#
#Prior probabilities of groups:
#NULL
#
#Misclassification rate:
# apparent: % In the list of attached packages vegan comes before klaR (like it should), but it seems that its Namespace was loaded before klaR Namespace, and attaching a package does not change this. I think that the design decision to make |
@brendanf : I confirm the
or alternative a Pull Request in github (against issue-#277 branch) or your confirmation that you don't want to be marked as the author of the commit. |
Fix incorrect argument assignment in plot.rda when called by ordiplot.
@jarioksa : quite interesting consequences. Other functions reference nullmodel objects as lists via the
would solve the issue. I will check/test. |
I looked at changing the nullmodel object to list instead of environment in my local copy (and in the meantime forgot to rebase my local repo and created an ugly looking merge...). Most things work fine. There is one fundamental issue though. The following code from the
That is because we keep updating the state of the objects inside of the environment ( It seems that for this reason, the environment was the right choice. Thoughts? |
See changes in branch |
@psolymos : yes, that it is. I was sure that we did not have it as a list just to be fancy, and that was the point. I remember we also inspected more radical solutions (like S4-classes), but updating sequential models was the key point. In that time we had a lot of trouble in simpler |
Having Very little can be done with |
This mitigates the problems with having functions vegan::rda and klaR::rda, and klaR with methods print.rda, plot.rda, and predict.rda. Now vegan functions should be used if vegan namespace is attached after klaR namespace (and vice versa), and if a method from a wrong package is used, an informative message should be given.
Added vegan:::print.rda that does nothing but delegates printing to NextMethod (i.e., vegan:::print.cca). Otherwise klaR:::print.rda may be used with obscure results. There is a similar name conflict with klaR:::plot.rda because vegan has no plot.rda function but delegates all plotting to vegan:::plot.cca. Addresses github issue #277. (cherry picked from commit feafe83)
Closed with commit 8f02d8b |
Package
klaR
(which is loaded by packageagricolae
) defines a classrda
with S3 methodsprint.rda()
andplot.rda()
. Even ifklaR
is not attached, it still registers the S3 methods when loaded.vegan
'srda
class inheritsplot.cca()
andprint.cca()
, but these are non-exported and so cannot be called directly. Furthermore,plot()
is called byordiplot()
, and this dispatches incorrectly on objects of typerda
, leading to an error.Suggested fix: create
plot.rda
andprint.rda
as aliases forplot.cca
andprint.rda
, and register the S3 methods.MnWE:
The text was updated successfully, but these errors were encountered: