Skip to content
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

Issues with R < 4.0 #107

Closed
josephwb opened this issue Sep 28, 2020 · 3 comments
Closed

Issues with R < 4.0 #107

josephwb opened this issue Sep 28, 2020 · 3 comments

Comments

@josephwb
Copy link

We were unable to install dispRity, whether from CRAN or github:

> install.packages("dispRity")
Installing package into ‘/home/josephwb/R/x86_64-pc-linux-gnu-library/3.6’
(as ‘lib’ is unspecified)
Warning in install.packages :
  package ‘dispRity’ is not available (for R version 3.6.3)

This was surprising, since no other phylo packages (that I am aware of) require such recent versions of R. For example:

    package R_version
1  phytools     3.5.0
2  phangorn     3.2.0
3    geiger      2.15
4      rotl     3.1.1
5       ape     3.2.0
6 paleotree     3.0.0
7     caper      2.10
8 BAMMtools      2.10

Changing the R version in the DESCRIPTION file to R (>= 3.5) revealed an odd issue with Rv4. During error-checking, dispRity checks the class of data. Weirdly, Rv4 has a different class for a matrix than does Rv3.*:

# generate a matrix
matrix(1:9, nrow=3, ncol=3) -> a;
b <- a;

# Rv4 gives the class above as:
# class(a);
# [1] "matrix" "array"
# while R v3.* gives:
# class(a);
# [1] "matrix"

Let's construct an example to demonstrate the problem:

# set the class attributes independent of R version:
class(a) <- "matrix";
class(b) <- c("matrix", "array");

The dispRity error check is essentially the following (where it is clear why things fail when there is only a single class attribute):

# the issue with Rv3.* was the following check:
all(class(a) == c("matrix", "array"))
# [1] FALSE
# while in Rv4:
all(class(b) == c("matrix", "array"))
# [1] TRUE

However, we can make a general check that works for both R versions:

# we can make this work for both versions of R with the following:
all(class(a) %in% c("matrix", "array"))
# [1] TRUE
all(class(b) %in% c("matrix", "array"))
# [1] TRUE

Yay! This is currently implemented in my fork. However, will submit a PR as it is a bit of a pain to update R itself just to install one package (^_-)≡☆

@josephwb josephwb mentioned this issue Sep 28, 2020
@dwbapst
Copy link
Contributor

dwbapst commented Sep 29, 2020

It might be equivalent, but I think current best practice is to use inherits instead, for example:

> x <- matrix(1:9, 3, 3)
> inherits(x, "matrix")
[1] TRUE

The change from using only 'matrix' for a 'matrix' to also giving it class 'array' was telegraphed a while ago. I think package authors received notice via email from CRAN... ah, yes, Dec 4th from Kurt H.:

Dear maintainer,

Please see the problems shown on
https://cran.r-project.org/web/checks/check_results_paleotree.html.

Specifically, see the problems shown for the r-devel Debian checks.

These can be reproduced by checking with --as-cran using current
r-devel, which for now sets

R_CLASS_MATRIX_ARRAY=true

in the check environment, to the effect that

R> class(matrix(1 : 4, 2, 2))
[1] "matrix" "array"

(and no longer just "matrix" as before).

According to the R NEWS file,

For now only active when environment variable R_CLASS_MATRIX_ARRAY
is set to non-empty, but planned to be the new unconditional behavior
when R 4.0.0 is released:

matrix objects now also inherit from class "array", namely, e.g.,
class(diag(1)) is c("matrix", "array") which invalidates code
assuming that length(class(obj)) == 1, an incorrect assumption that
is less frequently fulfilled now.

S3 methods for "array", i.e., .array(), are now also
dispatched for matrix objects.

Apparently your package no longer works correctly when
class(matrix(...)) gives a vector of length two: please fix as
necessary.

See
https://developer.r-project.org/Blog/public/2019/11/09/when-you-think-class.-think-again/index.html
for more information about correctly using class() in package code.

Please correct before 2019-12-18 to safely retain your package on CRAN.

Best,
-k

Well, apparently it was telegraphed to those of us for whom it broke our packages on CRAN...

@TGuillerme
Copy link
Owner

The R_CLASS_MATRIX_ARRAY changes for Rv4 were implemented in dispRity 1.4 (https://github.com/TGuillerme/dispRity/blob/master/NEWS.md) that I think was running fine for Rv3 users (i.e. I wasn't aware of any issue). The newest version (dispRity 1.5) now has more checks for the class of the input for the new functionalities that were the cause of version change.

I've taken the change from @josephwb credited in the NEWS.md and reverted the version number to Rv1.6.

This is now deployed on the master and release branches so that

devtools::install_github("TGuillerme/dispRity", ref = "release")

or

devtools::install_github("TGuillerme/dispRity")

Should work for older R versions (@josephwb, please let me know if it's the case so that I can close this issue).

@josephwb
Copy link
Author

Installed and running. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants