friendly / matlib

Matrix Functions for Teaching and Learning Linear Algebra and Multivariate Statistics
http://friendly.github.io/matlib/
65 stars 16 forks source link

new warnings in examples #21

Closed friendly closed 7 years ago

friendly commented 7 years ago

After creating the pkgdown pages, I see that a number of examples now generate warnings. E.g., example(Eigen) gives

....
Eigen> EC$vectors %*% diag(EC$values) %*% t(EC$vectors) # check
    [,1] [,2] [,3]
[1,]    1    2    3
[2,]    2    5    6
[3,]    3    6   10
There were 24 warnings (use warnings() to see them)

where the warning is

> warnings()
Warning messages:
1: In (X[, j] %*% E[, k]) * E[, k] :
  Recycling array of length 1 in array-vector arithmetic is deprecated.
  Use c() or as.vector() instead.

These should be tracked down and if possible fixed.

friendly commented 7 years ago

Here is a list of the docs/reference/*.html files containing "Warninng:". Some of these may be false positives.

C:\Users\friendly\Dropbox\R\projects\matlib\docs\reference>grep -m 1 -l Warning:
 *.html
Det.html
Eigen.html
LU.html
QR.html
R.html
SVD.html
arc.html
plot.regvec3d.html
regvec3d.html
showEig.html
john-d-fox commented 7 years ago

Hi Michael,

Some of this is probably in code I wrote. I'm currently out of town, returning today, and will take a look at it tomorrow.

Best, John

-----Original Message----- From: Michael Friendly [mailto:notifications@github.com] Sent: Monday, October 23, 2017 9:51 PM To: friendly/matlib matlib@noreply.github.com Cc: Subscribed subscribed@noreply.github.com Subject: Re: [friendly/matlib] new warnings (#21)

Here is a list of the docs/reference/*.html files containing "Warninng:". Some of these may be false positives.

C:\Users\friendly\Dropbox\R\projects\matlib\docs\reference>grep -m 1 -l Warning: *.html Det.html Eigen.html LU.html QR.html R.html SVD.html arc.html plot.regvec3d.html regvec3d.html showEig.html

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/friendly/matlib/issues/21#issuecomment-338847278 , or mute the thread https://github.com/notifications/unsubscribe- auth/ANcgQsDASvbfJTI27jY0r2NeXZAdzZoFks5svUKIgaJpZM4QDuJD . https://github.com/notifications/beacon/ANcgQlNke84XI_msudxHWo- eB7as_RBmks5svUKIgaJpZM4QDuJD.gif

philchalmers commented 7 years ago

I noticed this recently as well, and patch one of them in this commit: https://github.com/friendly/matlib/commit/252f64601d2696bdaebfb1bbbe24b51231357ef1

I'll try extracting the working examples and put them into a testthat block sometime soon, where even warnings can be turned into error messages when calling R CMD check automatically.

john-d-fox commented 7 years ago

Hi Phil,

Why not just redefine matrix multiplication (internally in the package)?

v <- 1:5 X <- matrix(1, 3, 3)

(v %% v) X[, 1] [1] 55 55 55 Warning message: In (v %% v) X[, 1] : Recycling array of length 1 in array-vector arithmetic is deprecated. Use c() or as.vector() instead.

%*% <- function(x, y){

  • z <- base::%*%(x, y)
  • if (all(dim(z) == c(1, 1))) z <- as.vector(z)
  • z
  • }

(v %% v) X[, 1] [1] 55 55 55

Best, John

-----Original Message----- From: Phil Chalmers [mailto:notifications@github.com] Sent: Tuesday, October 24, 2017 2:13 PM To: friendly/matlib matlib@noreply.github.com Cc: Fox, John jfox@mcmaster.ca; Comment comment@noreply.github.com Subject: Re: [friendly/matlib] new warnings in examples (#21)

I noticed this recently as well, and patch one of them in this commit: 252f646 https://github.com/friendly/matlib/commit/252f64601d2696bdaebfb1bbbe2 4b51231357ef1

I'll try extracting the working examples and put them into a testthat block sometime soon, where even warnings can be turned into error messages when calling R CMD check automatically.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/friendly/matlib/issues/21#issuecomment-339081953 , or mute the thread https://github.com/notifications/unsubscribe- auth/ANcgQn1p23NvtiybotE6XUTRKGiOy1Dmks5sviiUgaJpZM4QDuJD . https://github.com/notifications/beacon/ANcgQgvzZM3nPMHJ1TCs6xfCc5O9 Q0Mlks5sviiUgaJpZM4QDuJD.gif

friendly commented 7 years ago

That is very clever and seems like it would work, at least for top-level expressions in examples; maybe also in functions. Before committing to this, I'd like to track it down a bit further.

In this particular case, it might be simpler just to use c(),

 > c(v %*% v) * X[, 1]
[1] 55 55 55
philchalmers commented 7 years ago

I find this approach is pretty amusing and entertaining, but do we want to be those people who start redefining base R operations to break new conventions? I mean, R formally had this exact behaviour, but now it doesn't, and if matlib is loaded first then users may not become aware of this warning message until a new session is loaded.

I tend to lean on the side of fixing problems as R evolves rather than restore older definitions. If the %*% approach was not exported I could probably convinced otherwise, but changing the definition publicly by exporting/attaching makes me feel uneasy..... Just my 5 cents.

john-d-fox commented 7 years ago

Hi,

To be clear, it wasn't my intention to export the redefined %*%, just to use it internally in the package.

I just got back to town and haven't had time to see where in the package the warnings are occurring. If they are in code that's directly in examples, then it would still be necessary to use c() or as.vector(). The latter is harder to type but more expressive of what's being done. as.scalar() would be even better, but R doesn't have scalars.

I assumed that the offending code was in functions in the package; if there are no such instances, then my "solution" isn't appropriate. As for whether it's appropriate for internal use in the package, I'd argue that it is, unless there's some instance where we don't want to treat a 1x1 matrix as a scalar (in the matrix-algebra sense).

John

-----Original Message----- From: Phil Chalmers [mailto:notifications@github.com] Sent: Tuesday, October 24, 2017 5:24 PM To: friendly/matlib matlib@noreply.github.com Cc: Fox, John jfox@mcmaster.ca; Comment comment@noreply.github.com Subject: Re: [friendly/matlib] new warnings in examples (#21)

I find this approach is pretty amusing and entertaining, but do we want to be those people who start redefining base R operations to break new conventions? I mean, R formally had this exact behaviour, but now it doesn't, and if matlib is loaded first then users may not become aware of this warning message until a new session is loaded.

I tend to lean on the side of fixing problems as R evolves rather than restore older definitions. If the %*% approach was not exported I could probably convinced otherwise, but changing the definition publicly by exporting/attaching makes me feel uneasy..... Just my 5 cents.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/friendly/matlib/issues/21#issuecomment-339136406 , or mute the thread https://github.com/notifications/unsubscribe- auth/ANcgQqXVkaCJUoI50XFD-cleldAQQJx-ks5svlWLgaJpZM4QDuJD . https://github.com/notifications/beacon/ANcgQh3HsPIdudRyVlLbbHNZL1xM ANoPks5svlWLgaJpZM4QDuJD.gif

friendly commented 7 years ago

@philchalmers : You got similar issues in mirt, https://github.com/philchalmers/mirt/issues/108. How did you solve those?

philchalmers commented 7 years ago

Hi John,

Thanks for clarifying, sounds like we're on the same page. Looks like we're in for a warning hunt to find out the culprits (though now I'm tempted to define an as.scalar function.....).

philchalmers commented 7 years ago

@friendly I used as.vector()

friendly commented 7 years ago

A lot of these are traced to line 45 in QR.R, which now reads:

      U[, j] <- U[, j] - as.vector(X[, j] %*% E[, k]) * E[, k]

With this change, most of these warnings disappear. The remaining ones are in:

>grep -m 1 -l Warning:  *.html
LU.html
arc.html
plot.regvec3d.html
regvec3d.html
john-d-fox commented 7 years ago

Hi,

The potential downside of fixing these lines of code individually is the implicit assumption that the current examples reveal all possible situations that produce a 1 x 1 matrix involved in a scalar product. Maybe that’s the case. Alternatively, we could try to find all obvious 1 x 1 products produced as inner products of single rows and/or columns and/or vectors, or we could just redefine %*%.

Best, John

-----Original Message----- From: Michael Friendly [mailto:notifications@github.com] Sent: Tuesday, October 24, 2017 8:38 PM To: friendly/matlib matlib@noreply.github.com Cc: Fox, John jfox@mcmaster.ca; Comment comment@noreply.github.com Subject: Re: [friendly/matlib] new warnings in examples (#21)

A lot of these are traced to line 45 in QR.R, which now reads:

  U[, j] <- U[, j] - as.vector(X[, j] %*% E[, k]) * E[, k]

With this change, most of these warnings disappear. The remaining ones are in:

grep -m 1 -l Warning: *.html LU.html arc.html plot.regvec3d.html regvec3d.html

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/friendly/matlib/issues/21#issuecomment-339177707 , or mute the thread https://github.com/notifications/unsubscribe- auth/ANcgQjK8wKSKm48aGoh07-Ykt0SMl3L-ks5svoLpgaJpZM4QDuJD . https://github.com/notifications/beacon/ANcgQv-tMeosdtxsoVDuIC8Prfgz- 2bdks5svoLpgaJpZM4QDuJD.gif

philchalmers commented 7 years ago

@friendly some of these new warning messages look bizarre to me. I just ran some examples in LU(), and now see this output:

 LU(A, b, verbose=TRUE)

Initial equation:
 2*x1 + x2   - x3  =    8 
-3*x1 - x2 + 2*x3  =  -11 
-2*x1 + x2 + 2*x3  =   -3 

Lower triangle equation:
     x1              =    8 
-1.5*x1   + x2       =  -11 
    -x1 + 4*x2 + x3  =   -3 

Forward-solving operations:

  Equation: x1  =  8 
  Solution: x1 = 8

  Equation: -1.5*x1 + x2  =  -11 
  Substitution: -1.5*8 + x2 + 0*x3  =  -11 
  Solution: x2 = (-11 - -12) = 1

  Equation: -x1 + 4*x2 + x3  =  -3 
  Substitution: -8 + 4*1 + x3  =  -3 
  Solution: x3 = (-3 - -4) = 1

Intermediate solution: d = (8, 1, 1)

Upper triangle equation:
 Show Traceback

 Rerun with Debug
 Error in if (class(A) == "lm") { : 
  (converted from warning) the condition has length > 1 and only the first element will be used 

which as far as I can tell is being traced somehow from showEqn().

philchalmers commented 7 years ago

I see it now. In showEqn() the call if(class(A) == 'lm') no longer works because of R's new lazy evaluation convention. It fails when class has more than 1 element, so if() is receiving two or more logical inputs and doesn't really know what to do (I assume it just used the first by default before, without warning).

There must be more instances like this in the package now that throw this warning. The way I avoid this in my other packages is to just us is(A, 'someclass') instead of class(A) == 'someclass, because it always returns a single logical.

friendly commented 7 years ago

OK, I'll apply that to showEqn

john-d-fox commented 7 years ago

Hi,

As a general matter, using == to test class membership isn't a good idea; generally is() or inherits() is better. You have to be a bit careful if you want to check the first class of an S3 object. I know that I've been guilty of this before, so maybe I wrote the offending code.

Best, John

-----Original Message----- From: Phil Chalmers [mailto:notifications@github.com] Sent: Tuesday, October 24, 2017 9:22 PM To: friendly/matlib matlib@noreply.github.com Cc: Fox, John jfox@mcmaster.ca; Comment comment@noreply.github.com Subject: Re: [friendly/matlib] new warnings in examples (#21)

I see it now. In showEqn() the call if(class(A) == 'lm') no longer works because of R's new lazy evaluation convention. It fails when class has more than 1 element, so if() is receiving two or more logical inputs and doesn't really know what to do (I assume it just used the first by default before, without warning).

There must be more instances like this in the package now that throw this warning. The way I avoid this in my other packages is to just us is(A, 'someclass') instead of class(A) == 'someclass, because it always returns a single logical.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/friendly/matlib/issues/21#issuecomment-339184301 , or mute the thread https://github.com/notifications/unsubscribe- auth/ANcgQirU8lVu2sQHpdId4opWdKvhIjfnks5svo0ygaJpZM4QDuJD . https://github.com/notifications/beacon/ANcgQlJdTndbmDcCcuiWJuXG9dZO ANHkks5svo0ygaJpZM4QDuJD.gif

friendly commented 7 years ago

All warnings gone after latest build_site()!