R devel - May 2017 - stopifnot() does not stop at first non-TRUE argument

Le 15/05/2017 ? 15:37, Martin Maechler a ?crit :
>>>>>> Serguei Sokol <sokol at insa-toulouse.fr>
>>>>>>      on Mon, 15 May 2017 13:14:34 +0200 writes:
>      > I see in the archives that the attachment cannot pass.
>      > So, here is the code:
>
>      [....... MM: I needed to reformat etc to match closely to
>       the current source code which is in
>       	 https://svn.r-project.org/R/trunk/src/library/base/R/stop.R
>       or its corresponding github mirror
>         
https://github.com/wch/r-source/blob/trunk/src/library/base/R/stop.R
>      ]
>
>      > Best,
>      > Serguei.
>
> Yes, something like that seems even simpler than Peter's
> suggestion...
>
> It currently breaks 'make check' in the R sources,
> specifically in tests/reg-tests-2.R (lines 6574 ff),
> the new code now gives
>
>    > ## error messages from (C-level) evalList
>    > tst <- function(y) { stopifnot(is.numeric(y)); y+ 1 }
>    > try(tst())
>    Error in eval(cl.i, pfr) : argument "y" is missing, with no
default
>
> whereas previously it gave
>
>    Error in stopifnot(is.numeric(y)) :
>       argument "y" is missing, with no default
>
>
> But I think that change (of call stack in such an error case) is
> unavoidable and not a big problem.
It can be avoided but at price of customizing error() and warning() calls with
something like:
wrn <- function(w) {w$call <- cl.i; warning(w)}
err <- function(e) {e$call <- cl.i; stop(e)}
...
tryCatch(r <- eval(cl.i, pfr), warning=wrn, error=err)

Serguei.
>
> --
>
> I'm still curious about Herv?'s idea on using  switch()  for the
> issue.
>
> Martin
>
>
>      > Le 15/05/2017 ? 12:48, Serguei Sokol a ?crit :
>      >> Hello,
>      >>
>      >> I am a new on this list, so I introduce myself very briefly:
>      >> my background is applied mathematics, more precisely
scientific calculus
>      >> applied for modeling metabolic systems, I am
author/maintainer of
>      >> few packages (Deriv, rmumps, arrApply).
>      >>
>      >> Now, on the subject of this discussion, I must say that I
don't really understand
>      >> Peter's argument:
>      >>
>      >> >>> To do it differently, you would have to do
something like
>      >> >>>
>      >> >>> dots <- match.call(expand.dots=FALSE)$...
>      >> >>>
>      >> >>> and then explicitly evaluate each argument in
turn in the caller
>      >> >>> frame. This amount of nonstandard evaluation
sounds like it would
>      >> >>> incur a performance penalty, which could be
undesirable.
>      >> The first line of the current stopifnot()
>      >> n <- length(ll <- list(...))
>      >> already evaluates _all_ of the arguments
>      >> in the caller frame. So to do the same only
>      >> on a part of them (till the first FALSE or NA occurs)
>      >> cannot be more penalizing than the current version, right?
>      >>
>      >> I attach here a slightly modified version called
stopifnot_new()
>      >> which works in accordance with the man page and
>      >> where there are only two additional calls: parent.frame() and
eval().
>      >> I don't think it can be considered as real performance
penalty
>      >> as the same or bigger amount of (implicit) evaluations was
>      >> already done in the current version:
>      >>
>      >>> source("stopifnot_new.R")
>      >>> stopifnot_new(3 == 5, as.integer(2^32), a <- 12)
>      >> Error: 3 == 5 is not TRUE
>      >>> a
>      >> Error: object 'a' not found
>      >>
>      >> Best,
>      >> Serguei.
>      >>
>      >>
>      >> Le 15/05/2017 ? 10:39, Martin Maechler a ?crit :
>      >>>>>>>> Herv? Pag?s <hpages at
fredhutch.org>
>      >>>>>>>> on Wed, 3 May 2017 12:08:26 -0700
writes:
>      >>> > On 05/03/2017 12:04 PM, Herv? Pag?s wrote:
>      >>> >> Not sure why the performance penalty of
nonstandard evaluation would
>      >>> >> be more of a concern here than for something
like switch().
>      >>>
>      >>> > which is actually a primitive. So it seems that
there is at least
>      >>> > another way to go than 'dots <-
match.call(expand.dots=FALSE)$...'
>      >>>
>      >>> > Thanks, H.
>      >>>
>      >>> >>
>      >>> >> If that can't/won't be fixed, what about
fixing the man page so it's
>      >>> >> in sync with the current behavior?
>      >>> >>
>      >>> >> Thanks, H.
>      >>>
>      >>> Being back from vacations,...
>      >>> I agree that something should be done here, if not to the
code than at
>      >>> least to the man page.
>      >>>
>      >>> For now, I'd like to look a bit longer into a
possible change to the function.
>      >>> Peter mentioned a NSE way to fix the problem and you
mentioned switch().
>      >>>
>      >>> Originally, stopifnot() was only a few lines of code and
meant to be
>      >>> "self-explaining" by just reading its
definition, and I really would like
>      >>> to not walk too much away from that original idea.
>      >>> How did you (Herve) think to use  switch()  here?
>      >>>
>      >>>
>      >>>
>      >>> >> On 05/03/2017 02:26 AM, peter dalgaard wrote:
>      >>> >>> The first line of stopifnot is
>      >>> >>>
>      >>> >>> n <- length(ll <- list(...))
>      >>> >>>
>      >>> >>> which takes ALL arguments and forms a list
of them. This implies
>      >>> >>> evaluation, so explains the effect that you
see.
>      >>> >>>
>      >>> >>> To do it differently, you would have to do
something like
>      >>> >>>
>      >>> >>> dots <- match.call(expand.dots=FALSE)$...
>      >>> >>>
>      >>> >>> and then explicitly evaluate each argument
in turn in the caller
>      >>> >>> frame. This amount of nonstandard evaluation
sounds like it would
>      >>> >>> incur a performance penalty, which could be
undesirable.
>      >>> >>>
>      >>> >>> If you want to enforce the order of
evaluation, there is always
>      >>> >>>
>      >>> >>> stopifnot(A) stopifnot(B)
>      >>> >>>
>      >>> >>> -pd
>      >>> >>>
>      >>> >>>> On 3 May 2017, at 02:50 , Herv? Pag?s
<hpages at fredhutch.org>
>      >>> >>>> wrote:
>      >>> >>>>
>      >>> >>>> Hi,
>      >>> >>>>
>      >>> >>>> It's surprising that stopifnot()
keeps evaluating its arguments
>      >>> >>>> after it reaches the first one that is
not TRUE:
>      >>> >>>>
>      >>> >>>> > stopifnot(3 == 5, as.integer(2^32),
a <- 12) Error: 3 == 5 is
>      >>> >>>> not TRUE In addition: Warning message:
In stopifnot(3 == 5,
>      >>> >>>> as.integer(2^32), a <- 12) : NAs
introduced by coercion to integer
>      >>> >>>> range > a [1] 12
>      >>> >>>>
>      >>> >>>> The details section in its man page
actually suggests that it
>      >>> >>>> should stop at the first non-TRUE
argument:
>      >>> >>>>
>      >>> >>>> ?stopifnot(A, B)? is conceptually
equivalent to
>      >>> >>>>
>      >>> >>>> { if(any(is.na(A)) || !all(A))
stop(...); if(any(is.na(B)) ||
>      >>> >>>> !all(B)) stop(...) }
>      >>> >>>>
>      >>> >>>> Best, H.
>      >>> >>>>
>      >>> >>>> --
>      >>> >>>> Herv? Pag?s
>      >>> >>>>
>      >>> >>>> Program in Computational Biology
Division of Public Health
>      >>> >>>> Sciences Fred Hutchinson Cancer Research
Center 1100 Fairview
>      >>> >>>> Ave. N, M1-B514 P.O. Box 19024 Seattle,
WA 98109-1024
>      >>> >>>>
>      >>> >>>> E-mail: hpages at fredhutch.org Phone:
(206) 667-5791 Fax: (206)
>      >>> >>>> 667-1319
>      >>> >>>>
>      >>> >>>>
______________________________________________
>      >>> >>>> R-devel at r-project.org mailing list
>      >>> >>>>
>      >>>
https://urldefense.proofpoint.com/v2/url?u=https-3A__stat.ethz.ch_mailman_listinfo_r-2Ddevel&d=DwIFaQ&c=eRAMFD45gAfqt84VtBcfhQ&r=BK7q3XeAvimeWdGbWY_wJYbW0WYiZvSXAJJKaaPhzWA&m=JwgKhKD2k-9Kedeh6pqu-A8x6UEV0INrcxcSGVGo3Tg&s=f7IKJIhpRNJMC3rZAkuI6-MTdL3GAKSV2wK0boFN5HY&e>
>>> >>>>
>      >>> >>>
>      >>> >>
>      >>>
>      >>> > -- Herv? Pag?s
>      >>>
>      >>> > Program in Computational Biology Division of Public
Health Sciences
>      >>> > Fred Hutchinson Cancer Research Center 1100 Fairview
Ave. N,
>      >>> > M1-B514 P.O. Box 19024 Seattle, WA 98109-1024
>      >>>
>      >>> > E-mail: hpages at fredhutch.org Phone: (206)
667-5791 Fax: (206)
>      >>> > 667-1319
>      >>>
>      >>> > ______________________________________________
>      >>> > R-devel at r-project.org mailing list
>      >>> > https://stat.ethz.ch/mailman/listinfo/r-devel
>      >>>
>      >>> ______________________________________________
>      >>> R-devel at r-project.org mailing list
>      >>> https://stat.ethz.ch/mailman/listinfo/r-devel
>      >>
>
>      > ______________________________________________
>      > R-devel at r-project.org mailing list
>      > https://stat.ethz.ch/mailman/listinfo/r-devel
>

>>>>> Serguei Sokol <sokol at insa-toulouse.fr>
>>>>>     on Mon, 15 May 2017 16:32:20 +0200 writes:
    > Le 15/05/2017 ? 15:37, Martin Maechler a ?crit :
    >>>>>>> Serguei Sokol <sokol at insa-toulouse.fr>
    >>>>>>> on Mon, 15 May 2017 13:14:34 +0200 writes:
    >> > I see in the archives that the attachment cannot pass.
    >> > So, here is the code:
    >> 
    >> [....... MM: I needed to reformat etc to match closely to
    >> the current source code which is in
    >> https://svn.r-project.org/R/trunk/src/library/base/R/stop.R
    >> or its corresponding github mirror
    >>
https://github.com/wch/r-source/blob/trunk/src/library/base/R/stop.R
    >> ]
    >> 
    >> > Best,
    >> > Serguei.
    >> 
    >> Yes, something like that seems even simpler than Peter's
    >> suggestion...
    >> 
    >> It currently breaks 'make check' in the R sources,
    >> specifically in tests/reg-tests-2.R (lines 6574 ff),
    >> the new code now gives
    >> 
    >> > ## error messages from (C-level) evalList
    >> > tst <- function(y) { stopifnot(is.numeric(y)); y+ 1 }
    >> > try(tst())
    >> Error in eval(cl.i, pfr) : argument "y" is missing, with
no default
    >> 
    >> whereas previously it gave
    >> 
    >> Error in stopifnot(is.numeric(y)) :
    >> argument "y" is missing, with no default
    >> 
    >> 
    >> But I think that change (of call stack in such an error case) is
    >> unavoidable and not a big problem.

    > It can be avoided but at price of customizing error() and warning()
calls with something like:
    > wrn <- function(w) {w$call <- cl.i; warning(w)}
    > err <- function(e) {e$call <- cl.i; stop(e)}
    > ...
    > tryCatch(r <- eval(cl.i, pfr), warning=wrn, error=err)

    > Serguei.

Well, a good idea, but the 'warning' case is more complicated
(and the above incorrect): I do want the warning there, but
_not_ return the warning, but rather, the result of eval() :
So this needs even more sophistication, using  withCallingHandlers(.)
and maybe that really get's too sophisticated and no
more "readable" to 99.9% of the R users ... ?

I now do append my current version -- in case some may want to
comment or improve further.

Martin

-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: stopifnot_new.R.~3~
URL:
<https://stat.ethz.ch/pipermail/r-devel/attachments/20170515/4d595547/attachment.ksh>

Le 15/05/2017 ? 17:44, Martin Maechler a ?crit :
...
> So this needs even more sophistication, using withCallingHandlers(.)
> and maybe that really get's too sophisticated and no
> more "readable" to 99.9% of the R users ... ?
I'd say the current version is of minimal sophistication to reach
both the doc and test requirements.

Serguei.

This is getting pretty convoluted.

The current behavior is consistent with the description at the top of
the help page -- it does not promise to stop evaluation once the first
non-TRUE is found.  That seems OK to me -- if you want sequencing you
can use

stopifnot(A)
stopifnot(B)

or

stopifnot(A && B)

I could see an argument for a change that in the multiple argumetn
case reports _all_ that fail; that would seem more useful to me than
twisting the code into knots.

Best,

luke

On Mon, 15 May 2017, Martin Maechler wrote:
>>>>>> Serguei Sokol <sokol at insa-toulouse.fr>
>>>>>>     on Mon, 15 May 2017 16:32:20 +0200 writes:
>
>    > Le 15/05/2017 ? 15:37, Martin Maechler a ?crit :
>    >>>>>>> Serguei Sokol <sokol at
insa-toulouse.fr>
>    >>>>>>> on Mon, 15 May 2017 13:14:34 +0200 writes:
>    >> > I see in the archives that the attachment cannot pass.
>    >> > So, here is the code:
>    >>
>    >> [....... MM: I needed to reformat etc to match closely to
>    >> the current source code which is in
>    >> https://svn.r-project.org/R/trunk/src/library/base/R/stop.R
>    >> or its corresponding github mirror
>    >>
https://github.com/wch/r-source/blob/trunk/src/library/base/R/stop.R
>    >> ]
>    >>
>    >> > Best,
>    >> > Serguei.
>    >>
>    >> Yes, something like that seems even simpler than Peter's
>    >> suggestion...
>    >>
>    >> It currently breaks 'make check' in the R sources,
>    >> specifically in tests/reg-tests-2.R (lines 6574 ff),
>    >> the new code now gives
>    >>
>    >> > ## error messages from (C-level) evalList
>    >> > tst <- function(y) { stopifnot(is.numeric(y)); y+ 1 }
>    >> > try(tst())
>    >> Error in eval(cl.i, pfr) : argument "y" is missing,
with no default
>    >>
>    >> whereas previously it gave
>    >>
>    >> Error in stopifnot(is.numeric(y)) :
>    >> argument "y" is missing, with no default
>    >>
>    >>
>    >> But I think that change (of call stack in such an error case)
is
>    >> unavoidable and not a big problem.
>
>    > It can be avoided but at price of customizing error() and warning()
calls with something like:
>    > wrn <- function(w) {w$call <- cl.i; warning(w)}
>    > err <- function(e) {e$call <- cl.i; stop(e)}
>    > ...
>    > tryCatch(r <- eval(cl.i, pfr), warning=wrn, error=err)
>
>    > Serguei.
>
> Well, a good idea, but the 'warning' case is more complicated
> (and the above incorrect): I do want the warning there, but
> _not_ return the warning, but rather, the result of eval() :
> So this needs even more sophistication, using  withCallingHandlers(.)
> and maybe that really get's too sophisticated and no
> more "readable" to 99.9% of the R users ... ?
>
> I now do append my current version -- in case some may want to
> comment or improve further.
>
> Martin
>
>
-- 
Luke Tierney
Ralph E. Wareham Professor of Mathematical Sciences
University of Iowa                  Phone:             319-335-3386
Department of Statistics and        Fax:               319-335-3017
    Actuarial Science
241 Schaeffer Hall                  email:   luke-tierney at uiowa.edu
Iowa City, IA 52242                 WWW:  http://www.stat.uiowa.edu