Bug report: override stopifnot() ?

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

Bug report: override stopifnot() ?

Stepan Kasal
Hello,
I stumbled over a problem:
   stopifnot(m1 == m2)

It works with vector or matrix, but does not work for classes from Matrix package.

In the source of stopifnot(), there is all(m1 == m2) that would just work,
but there is also is.logical(m1 == m2) that id FALSE.

Would it be possible if Matrix package redefined stopifnot() ?

(If there is a bug tracking database for package Matrix, I would be happy to insert this report there.)

Thank you very much for the package,
    Stepan Kasal

______________________________________________
[hidden email] mailing list -- To UNSUBSCRIBE and more, see
https://stat.ethz.ch/mailman/listinfo/r-help
PLEASE do read the posting guide http://www.R-project.org/posting-guide.html
and provide commented, minimal, self-contained, reproducible code.
Reply | Threaded
Open this post in threaded view
|

Re: Bug report: override stopifnot() ?

R help mailing list-2
Why don't you use
   stopifnot( all(m1 == m2) )
?

Bill Dunlap
TIBCO Software
wdunlap tibco.com

On Mon, Mar 12, 2018 at 8:15 AM, Stepan Kasal <[hidden email]> wrote:

> Hello,
> I stumbled over a problem:
>    stopifnot(m1 == m2)
>
> It works with vector or matrix, but does not work for classes from Matrix
> package.
>
> In the source of stopifnot(), there is all(m1 == m2) that would just work,
> but there is also is.logical(m1 == m2) that id FALSE.
>
> Would it be possible if Matrix package redefined stopifnot() ?
>
> (If there is a bug tracking database for package Matrix, I would be happy
> to insert this report there.)
>
> Thank you very much for the package,
>     Stepan Kasal
>
> ______________________________________________
> [hidden email] mailing list -- To UNSUBSCRIBE and more, see
> https://stat.ethz.ch/mailman/listinfo/r-help
> PLEASE do read the posting guide http://www.R-project.org/
> posting-guide.html
> and provide commented, minimal, self-contained, reproducible code.
>

        [[alternative HTML version deleted]]

______________________________________________
[hidden email] mailing list -- To UNSUBSCRIBE and more, see
https://stat.ethz.ch/mailman/listinfo/r-help
PLEASE do read the posting guide http://www.R-project.org/posting-guide.html
and provide commented, minimal, self-contained, reproducible code.
Reply | Threaded
Open this post in threaded view
|

Re: Bug report: override stopifnot() ?

Stepan Kasal
Hello,

On Mon, Mar 12, 2018 at 09:30:59AM -0700, William Dunlap wrote:
> Why don't you use
>    stopifnot( all(m1 == m2) )
> ?

good question.  Even though I use
   aseert np.all(m1 == m2)
when working with NumPy, I got accustomed to the "handy shortcut"
that I can omit all() with R vectors and matrices.

Then I got trapped with the thing I reported.

On a second thought, omitting all() might have been bad idea from
the beginning; I should rather write all() routinely.
(It also reminds me that all.equal() is the right one in most cases.)

Is it true that using stopifnot() with non-scalar is considerd bad style?

If yes, could be perhaps stopifnot() enhanced to issue a warning to
teach new users of R, at least when they start using library(Matrix)?

If not, then enhancing stopifnot() to handle the case may be a good idea.

I also noticed the following:

> a <- Matrix(1)
> stopifnot(a == a)
Error: a == a is not TRUE
> if(a==a)print(1)
Error in if (a == a) print(1) : argument is not interpretable as logical

Neither does work, but the first error message is much more confusing.

When thinking about it, stopifnot() should really issue a better error
message in this case.  Patch attached.  But I should perhaps send
it also to R-devel.

Stepan Kasal


> On Mon, Mar 12, 2018 at 8:15 AM, Stepan Kasal <[hidden email]> wrote:
>
> > Hello,
> > I stumbled over a problem:
> >    stopifnot(m1 == m2)
> >
> > It works with vector or matrix, but does not work for classes from Matrix
> > package.
> >
> > In the source of stopifnot(), there is all(m1 == m2) that would just work,
> > but there is also is.logical(m1 == m2) that id FALSE.
> >
> > Would it be possible if Matrix package redefined stopifnot() ?
> >
> > (If there is a bug tracking database for package Matrix, I would be happy
> > to insert this report there.)
> >
> > Thank you very much for the package,
> >     Stepan Kasal
> >
> > ______________________________________________
> > [hidden email] mailing list -- To UNSUBSCRIBE and more, see
> > https://stat.ethz.ch/mailman/listinfo/r-help
> > PLEASE do read the posting guide http://www.R-project.org/
> > posting-guide.html
> > and provide commented, minimal, self-contained, reproducible code.
> >

______________________________________________
[hidden email] mailing list -- To UNSUBSCRIBE and more, see
https://stat.ethz.ch/mailman/listinfo/r-help
PLEASE do read the posting guide http://www.R-project.org/posting-guide.html
and provide commented, minimal, self-contained, reproducible code.
Reply | Threaded
Open this post in threaded view
|

Re: Bug report: override stopifnot() ?

Stepan Kasal
Hello,
I'm sorry that I aswer to my own mail; I forgot to attach the patch.
Patch below,
   Stepan Kasal

On Mon, Mar 12, 2018 at 06:53:00PM +0100, Stepan Kasal wrote:
> When thinking about it, stopifnot() should really issue a better error
> message in this case.  Patch attached.  But I should perhaps send
> it also to R-devel.


--- stopifnot-orig.r 2018-03-12 18:49:01.439484100 +0100
+++ stopifnot.r 2018-03-12 18:48:55.721846700 +0100
@@ -1,16 +1,20 @@
-function (...)
+function (...)
 {
   n <- length(ll <- list(...))
-  if (n == 0L)
+  if (n == 0L)
     return(invisible())
   mc <- match.call()
-  for (i in 1L:n) if (!(is.logical(r <- ll[[i]]) && !anyNA(r) &&
+  for (i in 1L:n) if (!(is.logical(r <- ll[[i]]) && !anyNA(r) &&
     all(r))) {
     ch <- deparse(mc[[i + 1]], width.cutoff = 60L)
-    if (length(ch) > 1L)
+    if (length(ch) > 1L)
       ch <- paste(ch[1L], "....")
-    stop(sprintf(ngettext(length(r), "%s is not TRUE", "%s are not all TRUE"),
-      ch), call. = FALSE, domain = NA)
+    if (is.logical(r)) {
+      msg <- ngettext(length(r), "%s is not TRUE", "%s are not all TRUE")
+    } else {
+      msg <- gettext("%s is not of type \"logical\"")
+    }
+    stop(sprintf(msg, ch), call. = FALSE, domain = NA)
   }
   invisible()
 }

______________________________________________
[hidden email] mailing list -- To UNSUBSCRIBE and more, see
https://stat.ethz.ch/mailman/listinfo/r-help
PLEASE do read the posting guide http://www.R-project.org/posting-guide.html
and provide commented, minimal, self-contained, reproducible code.
Reply | Threaded
Open this post in threaded view
|

Re: Bug report: override stopifnot() ?

Bert Gunter-2
In reply to this post by Stepan Kasal
Please stop this line of queries/"suggestions/speculations and read the
relevant docs **carefully**.

For example, from ?"=="

"Note

Do not use == and != for tests, such as in if expressions, where you must
get a single TRUE or FALSE. Unless you are absolutely sure that nothing
unusual can happen, you should use the identical
<http://127.0.0.1:22171/help/library/base/help/identical> function instead.
"

So you have already violated that specific warning, which led to the
confusion you evidence. Specifically:
> Matrix(1)== Matrix(1)
1 x 1 Matrix of class "lsyMatrix"
     [,1]
[1,] TRUE

That is, the result is not a logical but a (S4) object of class "lsyMatrix"
that contains a logical. Whence your (expected) error message.

Cheers,

Bert





Bert Gunter

"The trouble with having an open mind is that people keep coming along and
sticking things into it."
-- Opus (aka Berkeley Breathed in his "Bloom County" comic strip )

On Mon, Mar 12, 2018 at 10:53 AM, Stepan Kasal <[hidden email]> wrote:

> Hello,
>
> On Mon, Mar 12, 2018 at 09:30:59AM -0700, William Dunlap wrote:
> > Why don't you use
> >    stopifnot( all(m1 == m2) )
> > ?
>
> good question.  Even though I use
>    aseert np.all(m1 == m2)
> when working with NumPy, I got accustomed to the "handy shortcut"
> that I can omit all() with R vectors and matrices.
>
> Then I got trapped with the thing I reported.
>
> On a second thought, omitting all() might have been bad idea from
> the beginning; I should rather write all() routinely.
> (It also reminds me that all.equal() is the right one in most cases.)
>
> Is it true that using stopifnot() with non-scalar is considerd bad style?
>
> If yes, could be perhaps stopifnot() enhanced to issue a warning to
> teach new users of R, at least when they start using library(Matrix)?
>
> If not, then enhancing stopifnot() to handle the case may be a good idea.
>
> I also noticed the following:
>
> > a <- Matrix(1)
> > stopifnot(a == a)
> Error: a == a is not TRUE
> > if(a==a)print(1)
> Error in if (a == a) print(1) : argument is not interpretable as logical
>
> Neither does work, but the first error message is much more confusing.
>
> When thinking about it, stopifnot() should really issue a better error
> message in this case.  Patch attached.  But I should perhaps send
> it also to R-devel.
>
> Stepan Kasal
>
>
> > On Mon, Mar 12, 2018 at 8:15 AM, Stepan Kasal <[hidden email]> wrote:
> >
> > > Hello,
> > > I stumbled over a problem:
> > >    stopifnot(m1 == m2)
> > >
> > > It works with vector or matrix, but does not work for classes from
> Matrix
> > > package.
> > >
> > > In the source of stopifnot(), there is all(m1 == m2) that would just
> work,
> > > but there is also is.logical(m1 == m2) that id FALSE.
> > >
> > > Would it be possible if Matrix package redefined stopifnot() ?
> > >
> > > (If there is a bug tracking database for package Matrix, I would be
> happy
> > > to insert this report there.)
> > >
> > > Thank you very much for the package,
> > >     Stepan Kasal
> > >
> > > ______________________________________________
> > > [hidden email] mailing list -- To UNSUBSCRIBE and more, see
> > > https://stat.ethz.ch/mailman/listinfo/r-help
> > > PLEASE do read the posting guide http://www.R-project.org/
> > > posting-guide.html
> > > and provide commented, minimal, self-contained, reproducible code.
> > >
>
> ______________________________________________
> [hidden email] mailing list -- To UNSUBSCRIBE and more, see
> https://stat.ethz.ch/mailman/listinfo/r-help
> PLEASE do read the posting guide http://www.R-project.org/
> posting-guide.html
> and provide commented, minimal, self-contained, reproducible code.
>

        [[alternative HTML version deleted]]

______________________________________________
[hidden email] mailing list -- To UNSUBSCRIBE and more, see
https://stat.ethz.ch/mailman/listinfo/r-help
PLEASE do read the posting guide http://www.R-project.org/posting-guide.html
and provide commented, minimal, self-contained, reproducible code.
Reply | Threaded
Open this post in threaded view
|

Re: Bug report: override stopifnot() ?

Duncan Murdoch-2
On 12/03/2018 2:43 PM, Bert Gunter wrote:

> Please stop this line of queries/"suggestions/speculations and read the
> relevant docs **carefully**.
>
> For example, from ?"=="
>
> "Note
>
> Do not use == and != for tests, such as in if expressions, where you must
> get a single TRUE or FALSE. Unless you are absolutely sure that nothing
> unusual can happen, you should use the identical
> <http://127.0.0.1:22171/help/library/base/help/identical> function instead.
> "

But stopifnot(expr) is not a test where you must get a single TRUE or
FALSE.  See the Details section on its help page, or read its
Description carefully (where "all" is used in the technical sense of the
all() function), ignoring the final few words which seem to suggest that
c(TRUE, TRUE) is not okay.

Duncan Murdoch

>
> So you have already violated that specific warning, which led to the
> confusion you evidence. Specifically:
>> Matrix(1)== Matrix(1)
> 1 x 1 Matrix of class "lsyMatrix"
>       [,1]
> [1,] TRUE
>
> That is, the result is not a logical but a (S4) object of class "lsyMatrix"
> that contains a logical. Whence your (expected) error message.
>
> Cheers,
>
> Bert
>
>
>
>
>
> Bert Gunter
>
> "The trouble with having an open mind is that people keep coming along and
> sticking things into it."
> -- Opus (aka Berkeley Breathed in his "Bloom County" comic strip )
>
> On Mon, Mar 12, 2018 at 10:53 AM, Stepan Kasal <[hidden email]> wrote:
>
>> Hello,
>>
>> On Mon, Mar 12, 2018 at 09:30:59AM -0700, William Dunlap wrote:
>>> Why don't you use
>>>     stopifnot( all(m1 == m2) )
>>> ?
>>
>> good question.  Even though I use
>>     aseert np.all(m1 == m2)
>> when working with NumPy, I got accustomed to the "handy shortcut"
>> that I can omit all() with R vectors and matrices.
>>
>> Then I got trapped with the thing I reported.
>>
>> On a second thought, omitting all() might have been bad idea from
>> the beginning; I should rather write all() routinely.
>> (It also reminds me that all.equal() is the right one in most cases.)
>>
>> Is it true that using stopifnot() with non-scalar is considerd bad style?
>>
>> If yes, could be perhaps stopifnot() enhanced to issue a warning to
>> teach new users of R, at least when they start using library(Matrix)?
>>
>> If not, then enhancing stopifnot() to handle the case may be a good idea.
>>
>> I also noticed the following:
>>
>>> a <- Matrix(1)
>>> stopifnot(a == a)
>> Error: a == a is not TRUE
>>> if(a==a)print(1)
>> Error in if (a == a) print(1) : argument is not interpretable as logical
>>
>> Neither does work, but the first error message is much more confusing.
>>
>> When thinking about it, stopifnot() should really issue a better error
>> message in this case.  Patch attached.  But I should perhaps send
>> it also to R-devel.
>>
>> Stepan Kasal
>>
>>
>>> On Mon, Mar 12, 2018 at 8:15 AM, Stepan Kasal <[hidden email]> wrote:
>>>
>>>> Hello,
>>>> I stumbled over a problem:
>>>>     stopifnot(m1 == m2)
>>>>
>>>> It works with vector or matrix, but does not work for classes from
>> Matrix
>>>> package.
>>>>
>>>> In the source of stopifnot(), there is all(m1 == m2) that would just
>> work,
>>>> but there is also is.logical(m1 == m2) that id FALSE.
>>>>
>>>> Would it be possible if Matrix package redefined stopifnot() ?
>>>>
>>>> (If there is a bug tracking database for package Matrix, I would be
>> happy
>>>> to insert this report there.)
>>>>
>>>> Thank you very much for the package,
>>>>      Stepan Kasal
>>>>
>>>> ______________________________________________
>>>> [hidden email] mailing list -- To UNSUBSCRIBE and more, see
>>>> https://stat.ethz.ch/mailman/listinfo/r-help
>>>> PLEASE do read the posting guide http://www.R-project.org/
>>>> posting-guide.html
>>>> and provide commented, minimal, self-contained, reproducible code.
>>>>
>>
>> ______________________________________________
>> [hidden email] mailing list -- To UNSUBSCRIBE and more, see
>> https://stat.ethz.ch/mailman/listinfo/r-help
>> PLEASE do read the posting guide http://www.R-project.org/
>> posting-guide.html
>> and provide commented, minimal, self-contained, reproducible code.
>>
>
> [[alternative HTML version deleted]]
>
> ______________________________________________
> [hidden email] mailing list -- To UNSUBSCRIBE and more, see
> https://stat.ethz.ch/mailman/listinfo/r-help
> PLEASE do read the posting guide http://www.R-project.org/posting-guide.html
> and provide commented, minimal, self-contained, reproducible code.
>

______________________________________________
[hidden email] mailing list -- To UNSUBSCRIBE and more, see
https://stat.ethz.ch/mailman/listinfo/r-help
PLEASE do read the posting guide http://www.R-project.org/posting-guide.html
and provide commented, minimal, self-contained, reproducible code.