all.equal applied to function closures

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

all.equal applied to function closures

R devel mailing list
Consider the following code:

    f <- function(x)function(y){x+y}
    all.equal(f(5), f(0))

This returns TRUE, when it should return FALSE; I think it’s hard to make the case that f(5) and f(0) are “approximately equal” in any meaningful sense. Digging into the code for all.equal(), I see that all.equal(f(5), f(0)) results in a call to all.equal.language(f(5), f(0)), which only compares the function texts for equality.

If it is decided to leave this behavior as-is, then at least it should be documented. Currently I cannot find any documentation for all.equal applied to functions.

        [[alternative HTML version deleted]]

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Reply | Threaded
Open this post in threaded view
|

Re: all.equal applied to function closures

Duncan Murdoch-2
On 30/11/2020 1:05 p.m., Kevin Van Horn via R-devel wrote:
> Consider the following code:
>
>      f <- function(x)function(y){x+y}
>      all.equal(f(5), f(0))
>
> This returns TRUE, when it should return FALSE; I think it’s hard to make the case that f(5) and f(0) are “approximately equal” in any meaningful sense. Digging into the code for all.equal(), I see that all.equal(f(5), f(0)) results in a call to all.equal.language(f(5), f(0)), which only compares the function texts for equality.
>
> If it is decided to leave this behavior as-is, then at least it should be documented. Currently I cannot find any documentation for all.equal applied to functions.

Clearly it should also compare the environments of the two functions,
then it would see a difference:

 > all.equal(environment(f(5)), environment(f(0)))
[1] "Component “x”: Mean relative difference: 1"

Changing the first few lines from

     if (is.language(target) || is.function(target))
         return(all.equal.language(target, current, ...))

to

     if (is.function(target)) {
         msg <- all.equal.language(target, current, ...)
         if (isTRUE(msg)) {
             msg <- all.equal.environment(environment(target),
environment(current), ...)
             if (is.character(msg))
               msg <- paste("Environments:", msg)
         }
         return(msg)
     }
     if (is.language(target))
         return(all.equal.language(target, current, ...))

would fix it.

Duncan Murdoch

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Reply | Threaded
Open this post in threaded view
|

Re: all.equal applied to function closures

Bill Dunlap-2
To make the comparison more complete, all.equal.environment could compare
the parents of the target and current environments.  That would have to be
recursive but could stop at the first 'top level environment' (the global,
empty, or a package-related environment generally) and use identical
there.  E.g.,

> f1 <- function(x) (function(){ expx <- exp(x) ; function(y) y + expx})()
> all.equal(f1(2), f1(3))
[1] "Environments: Component “expx”: Mean relative difference: 1.718282"

[2] "Environments: <parent.env> Component “x”: Mean relative difference:
0.5"

This is from the following, where I avoided putting the existing
non-recursive all.equal.environment into the body of this one.

all.equal.environment <-
function(target, current, ...)
{
    .all.equal.environment <- base::all.equal.environment # temporary hack
    stopifnot(is.environment(target), is.environment(current))
    if (identical(target, current)) {
        TRUE
    } else {
        msg <- NULL # TODO: check attributes
        # deal with emptyenv now since parent.env(emptyenv()) gives error
        # and topenv(emptyenv()) gives GlobalEnv
        eTarget <- identical(target, emptyenv()) ||
identical(target,topenv(target))
        eCurrent <- identical(current, emptyenv()) ||
identical(current,topenv(current))
        if (eTarget || eCurrent) {
            msg <- c(msg, paste("target is", format(target), "and current
is", format(current)))
        } else {
            thisComparison <- .all.equal.environment(target, current, ...)
            if (!isTRUE(thisComparison)) {
                msg <- c(msg, thisComparison)
            }
            parentComparison <- Recall(parent.env(target),
parent.env(current), ...)
            if (!isTRUE(parentComparison)) {
                msg <- c(msg, paste("<parent.env>", parentComparison))
            }
        }
        if (is.null(msg) || isTRUE(msg)) TRUE else msg
    }
}

On Mon, Nov 30, 2020 at 10:42 AM Duncan Murdoch <[hidden email]>
wrote:

> On 30/11/2020 1:05 p.m., Kevin Van Horn via R-devel wrote:
> > Consider the following code:
> >
> >      f <- function(x)function(y){x+y}
> >      all.equal(f(5), f(0))
> >
> > This returns TRUE, when it should return FALSE; I think it’s hard to
> make the case that f(5) and f(0) are “approximately equal” in any
> meaningful sense. Digging into the code for all.equal(), I see that
> all.equal(f(5), f(0)) results in a call to all.equal.language(f(5), f(0)),
> which only compares the function texts for equality.
> >
> > If it is decided to leave this behavior as-is, then at least it should
> be documented. Currently I cannot find any documentation for all.equal
> applied to functions.
>
> Clearly it should also compare the environments of the two functions,
> then it would see a difference:
>
>  > all.equal(environment(f(5)), environment(f(0)))
> [1] "Component “x”: Mean relative difference: 1"
>
> Changing the first few lines from
>
>      if (is.language(target) || is.function(target))
>          return(all.equal.language(target, current, ...))
>
> to
>
>      if (is.function(target)) {
>          msg <- all.equal.language(target, current, ...)
>          if (isTRUE(msg)) {
>              msg <- all.equal.environment(environment(target),
> environment(current), ...)
>              if (is.character(msg))
>                msg <- paste("Environments:", msg)
>          }
>          return(msg)
>      }
>      if (is.language(target))
>          return(all.equal.language(target, current, ...))
>
> would fix it.
>
> Duncan Murdoch
>
> ______________________________________________
> [hidden email] mailing list
> https://stat.ethz.ch/mailman/listinfo/r-devel
>

        [[alternative HTML version deleted]]

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Reply | Threaded
Open this post in threaded view
|

Re: all.equal applied to function closures

Martin Maechler
>>>>> Bill Dunlap
>>>>>     on Mon, 30 Nov 2020 13:41:54 -0800 writes:

    > To make the comparison more complete, all.equal.environment could compare
    > the parents of the target and current environments.  That would have to be
    > recursive but could stop at the first 'top level environment' (the global,
    > empty, or a package-related environment generally) and use identical
    > there.  E.g.,

    > > f1 <- function(x) (function(){ expx <- exp(x) ; function(y) y + expx})()
    > > all.equal(f1(2), f1(3))
    > [1] "Environments: Component “expx”: Mean relative difference: 1.718282"
    >
    > [2] "Environments: <parent.env> Component “x”: Mean relative difference:
    > 0.5"

    > This is from the following, where I avoided putting the existing
    > non-recursive all.equal.environment into the body of this one.

    > all.equal.environment <-
    > function(target, current, ...)
    > {
    >     .all.equal.environment <- base::all.equal.environment # temporary hack
    >     stopifnot(is.environment(target), is.environment(current))
    >     if (identical(target, current)) {
    >         TRUE
    >     } else {
    >         msg <- NULL # TODO: check attributes
    >         # deal with emptyenv now since parent.env(emptyenv()) gives error
    >         # and topenv(emptyenv()) gives GlobalEnv
    >         eTarget <- identical(target, emptyenv()) ||
    > identical(target,topenv(target))
    >         eCurrent <- identical(current, emptyenv()) ||
    > identical(current,topenv(current))
    >         if (eTarget || eCurrent) {
    >             msg <- c(msg, paste("target is", format(target), "and current
    > is", format(current)))
    >         } else {
    >             thisComparison <- .all.equal.environment(target, current, ...)
    >             if (!isTRUE(thisComparison)) {
    >                 msg <- c(msg, thisComparison)
    >             }
    >             parentComparison <- Recall(parent.env(target),
    > parent.env(current), ...)
    >             if (!isTRUE(parentComparison)) {
    >                 msg <- c(msg, paste("<parent.env>", parentComparison))
    >             }
    >         }
    >         if (is.null(msg) || isTRUE(msg)) TRUE else msg
    >     }
    > }

Thank you, Duncan and Bill (and Kevin for bringing up the
topic).

I agree  all.equal() should work better with functions,

and I think probably it would make sense to define  all.equal.function()
rather than put this into all.equal.default()

However, it's not quite clear if it is always desirable to check the
environments as well notably as that *is* done recursively.

Bill, I'm sure you've noticed that we did write  all.equal.environment()
to work recursively... Actually, I had worked quite a bit at
that, too long ago to remember details, but the relevant svn log
entry is
------------------------------------------------------------------------
r66640 | maechler | 2014-09-18 22:10:20 +0200 (Thu, 18 Sep 2014) | 1 line

more sophisticated all.equal.environment(): no longer "simple" infinite recursions
------------------------------------------------------------------------

Are you sure that code with the internal recursive do1()
function should/could not be amended where needed?

Martin

    > On Mon, Nov 30, 2020 at 10:42 AM Duncan Murdoch <[hidden email]>
    > wrote:
    >
    > > On 30/11/2020 1:05 p.m., Kevin Van Horn via R-devel wrote:
    > > > Consider the following code:
    > > >
    > > >      f <- function(x)function(y){x+y}
    > > >      all.equal(f(5), f(0))
    > > >
    > > > This returns TRUE, when it should return FALSE; I think it’s hard to
    > > make the case that f(5) and f(0) are “approximately equal” in any
    > > meaningful sense. Digging into the code for all.equal(), I see that
    > > all.equal(f(5), f(0)) results in a call to all.equal.language(f(5), f(0)),
    > > which only compares the function texts for equality.
    > > >
    > > > If it is decided to leave this behavior as-is, then at least it should
    > > be documented. Currently I cannot find any documentation for all.equal
    > > applied to functions.
    > >
    > > Clearly it should also compare the environments of the two functions,
    > > then it would see a difference:
    > >
    > >  > all.equal(environment(f(5)), environment(f(0)))
    > > [1] "Component “x”: Mean relative difference: 1"
    > >
    > > Changing the first few lines from
    > >
    > >      if (is.language(target) || is.function(target))
    > >          return(all.equal.language(target, current, ...))
    > >
    > > to
    > >
    > >      if (is.function(target)) {
    > >          msg <- all.equal.language(target, current, ...)
    > >          if (isTRUE(msg)) {
    > >              msg <- all.equal.environment(environment(target),
    > > environment(current), ...)
    > >              if (is.character(msg))
    > >                msg <- paste("Environments:", msg)
    > >          }
    > >          return(msg)
    > >      }
    > >      if (is.language(target))
    > >          return(all.equal.language(target, current, ...))
    > >
    > > would fix it.
    > >
    > > Duncan Murdoch
    > >
    > > ______________________________________________
    > > [hidden email] mailing list
    > > https://stat.ethz.ch/mailman/listinfo/r-devel

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Reply | Threaded
Open this post in threaded view
|

Re: all.equal applied to function closures

Bill Dunlap-2
Probably all.equal.environment's do1() could be enhanced to do the
recursion (and look at the environments' attributes).  I wrote a separate
function because it was easier to experiment that way (e.g., when to stop
recursing - it stops when one environment is a top-level environment or the
empty environment).  I've been thinking about similar recursion options for
ls.str() - it would make it easier to debug refClass and R6 code where the
data is somewhere in a stack of environments.

> E1 <- list2env(list(X=1.1, Y=1.2), parent=list2env(list(p=1.3),
parent=baseenv()))
> E2 <- list2env(list(X=1.1, Y=1.2), parent=list2env(list(p=1.4),
parent=baseenv()))
> base::all.equal.environment(E1,E2)
[1] TRUE
> globalenv()$all.equal.environment(E1,E2)
[1] "<parent.env> Component “p”: Mean relative difference: 0.07692308"
>
> E3 <- list2env(list(X=1.1, Y=1.2), parent=list2env(list(p=1.5),
parent=new.env(parent=baseenv())))
> base::all.equal.environment(E1,E3)
[1] TRUE
> globalenv()$all.equal.environment(E1,E3)
[1] "<parent.env> Component “p”: Mean relative difference: 0.1538462"
[2] "<parent.env> <parent.env> target is <environment: base> and current is
<environment: 0x564e806705c8>"

On Tue, Dec 1, 2020 at 1:31 AM Martin Maechler <[hidden email]>
wrote:

> >>>>> Bill Dunlap
> >>>>>     on Mon, 30 Nov 2020 13:41:54 -0800 writes:
>
>     > To make the comparison more complete, all.equal.environment could
> compare
>     > the parents of the target and current environments.  That would have
> to be
>     > recursive but could stop at the first 'top level environment' (the
> global,
>     > empty, or a package-related environment generally) and use identical
>     > there.  E.g.,
>
>     > > f1 <- function(x) (function(){ expx <- exp(x) ; function(y) y +
> expx})()
>     > > all.equal(f1(2), f1(3))
>     > [1] "Environments: Component “expx”: Mean relative difference:
> 1.718282"
>     >
>     > [2] "Environments: <parent.env> Component “x”: Mean relative
> difference:
>     > 0.5"
>
>     > This is from the following, where I avoided putting the existing
>     > non-recursive all.equal.environment into the body of this one.
>
>     > all.equal.environment <-
>     > function(target, current, ...)
>     > {
>     >     .all.equal.environment <- base::all.equal.environment #
> temporary hack
>     >     stopifnot(is.environment(target), is.environment(current))
>     >     if (identical(target, current)) {
>     >         TRUE
>     >     } else {
>     >         msg <- NULL # TODO: check attributes
>     >         # deal with emptyenv now since parent.env(emptyenv()) gives
> error
>     >         # and topenv(emptyenv()) gives GlobalEnv
>     >         eTarget <- identical(target, emptyenv()) ||
>     > identical(target,topenv(target))
>     >         eCurrent <- identical(current, emptyenv()) ||
>     > identical(current,topenv(current))
>     >         if (eTarget || eCurrent) {
>     >             msg <- c(msg, paste("target is", format(target), "and
> current
>     > is", format(current)))
>     >         } else {
>     >             thisComparison <- .all.equal.environment(target,
> current, ...)
>     >             if (!isTRUE(thisComparison)) {
>     >                 msg <- c(msg, thisComparison)
>     >             }
>     >             parentComparison <- Recall(parent.env(target),
>     > parent.env(current), ...)
>     >             if (!isTRUE(parentComparison)) {
>     >                 msg <- c(msg, paste("<parent.env>",
> parentComparison))
>     >             }
>     >         }
>     >         if (is.null(msg) || isTRUE(msg)) TRUE else msg
>     >     }
>     > }
>
> Thank you, Duncan and Bill (and Kevin for bringing up the
> topic).
>
> I agree  all.equal() should work better with functions,
>
> and I think probably it would make sense to define  all.equal.function()
> rather than put this into all.equal.default()
>
> However, it's not quite clear if it is always desirable to check the
> environments as well notably as that *is* done recursively.
>
> Bill, I'm sure you've noticed that we did write  all.equal.environment()
> to work recursively... Actually, I had worked quite a bit at
> that, too long ago to remember details, but the relevant svn log
> entry is
> ------------------------------------------------------------------------
> r66640 | maechler | 2014-09-18 22:10:20 +0200 (Thu, 18 Sep 2014) | 1 line
>
> more sophisticated all.equal.environment(): no longer "simple" infinite
> recursions
> ------------------------------------------------------------------------
>
> Are you sure that code with the internal recursive do1()
> function should/could not be amended where needed?
>
> Martin
>
>     > On Mon, Nov 30, 2020 at 10:42 AM Duncan Murdoch <
> [hidden email]>
>     > wrote:
>     >
>     > > On 30/11/2020 1:05 p.m., Kevin Van Horn via R-devel wrote:
>     > > > Consider the following code:
>     > > >
>     > > >      f <- function(x)function(y){x+y}
>     > > >      all.equal(f(5), f(0))
>     > > >
>     > > > This returns TRUE, when it should return FALSE; I think it’s
> hard to
>     > > make the case that f(5) and f(0) are “approximately equal” in any
>     > > meaningful sense. Digging into the code for all.equal(), I see that
>     > > all.equal(f(5), f(0)) results in a call to
> all.equal.language(f(5), f(0)),
>     > > which only compares the function texts for equality.
>     > > >
>     > > > If it is decided to leave this behavior as-is, then at least it
> should
>     > > be documented. Currently I cannot find any documentation for
> all.equal
>     > > applied to functions.
>     > >
>     > > Clearly it should also compare the environments of the two
> functions,
>     > > then it would see a difference:
>     > >
>     > >  > all.equal(environment(f(5)), environment(f(0)))
>     > > [1] "Component “x”: Mean relative difference: 1"
>     > >
>     > > Changing the first few lines from
>     > >
>     > >      if (is.language(target) || is.function(target))
>     > >          return(all.equal.language(target, current, ...))
>     > >
>     > > to
>     > >
>     > >      if (is.function(target)) {
>     > >          msg <- all.equal.language(target, current, ...)
>     > >          if (isTRUE(msg)) {
>     > >              msg <- all.equal.environment(environment(target),
>     > > environment(current), ...)
>     > >              if (is.character(msg))
>     > >                msg <- paste("Environments:", msg)
>     > >          }
>     > >          return(msg)
>     > >      }
>     > >      if (is.language(target))
>     > >          return(all.equal.language(target, current, ...))
>     > >
>     > > would fix it.
>     > >
>     > > Duncan Murdoch
>     > >
>     > > ______________________________________________
>     > > [hidden email] mailing list
>     > > https://stat.ethz.ch/mailman/listinfo/r-devel
>

        [[alternative HTML version deleted]]

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Reply | Threaded
Open this post in threaded view
|

Re: all.equal applied to function closures

Duncan Murdoch-2
One more comment:  it might be worthwhile reporting on a case where
identical(e1, e2) fails when those are the environments associated with
two functions (though I think not by default in all.equal.environment).

Functions can modify variables in their environment, so examples like
the open.account example in the R-intro manual would distinguish between
ross and robert even if the balances matched.

Duncan Murdoch

On 01/12/2020 10:37 a.m., Bill Dunlap wrote:

> Probably all.equal.environment's do1() could be enhanced to do the
> recursion (and look at the environments' attributes).  I wrote a
> separate function because it was easier to experiment that way (e.g.,
> when to stop recursing - it stops when one environment is a top-level
> environment or the empty environment).  I've been thinking about similar
> recursion options for ls.str() - it would make it easier to debug
> refClass and R6 code where the data is somewhere in a stack of environments.
>
>  > E1 <- list2env(list(X=1.1, Y=1.2), parent=list2env(list(p=1.3),
> parent=baseenv()))
>  > E2 <- list2env(list(X=1.1, Y=1.2), parent=list2env(list(p=1.4),
> parent=baseenv()))
>  > base::all.equal.environment(E1,E2)
> [1] TRUE
>  > globalenv()$all.equal.environment(E1,E2)
> [1] "<parent.env> Component “p”: Mean relative difference: 0.07692308"
>  >
>  > E3 <- list2env(list(X=1.1, Y=1.2), parent=list2env(list(p=1.5),
> parent=new.env(parent=baseenv())))
>  > base::all.equal.environment(E1,E3)
> [1] TRUE
>  > globalenv()$all.equal.environment(E1,E3)
> [1] "<parent.env> Component “p”: Mean relative difference: 0.1538462"
> [2] "<parent.env> <parent.env> target is <environment: base> and current
> is <environment: 0x564e806705c8>"
>
> On Tue, Dec 1, 2020 at 1:31 AM Martin Maechler
> <[hidden email] <mailto:[hidden email]>> wrote:
>
>      >>>>> Bill Dunlap
>      >>>>>     on Mon, 30 Nov 2020 13:41:54 -0800 writes:
>
>          > To make the comparison more complete, all.equal.environment
>     could compare
>          > the parents of the target and current environments.  That
>     would have to be
>          > recursive but could stop at the first 'top level environment'
>     (the global,
>          > empty, or a package-related environment generally) and use
>     identical
>          > there.  E.g.,
>
>          > > f1 <- function(x) (function(){ expx <- exp(x) ; function(y)
>     y + expx})()
>          > > all.equal(f1(2), f1(3))
>          > [1] "Environments: Component “expx”: Mean relative
>     difference: 1.718282"
>          >
>          > [2] "Environments: <parent.env> Component “x”: Mean relative
>     difference:
>          > 0.5"
>
>          > This is from the following, where I avoided putting the existing
>          > non-recursive all.equal.environment into the body of this one.
>
>          > all.equal.environment <-
>          > function(target, current, ...)
>          > {
>          >     .all.equal.environment <- base::all.equal.environment #
>     temporary hack
>          >     stopifnot(is.environment(target), is.environment(current))
>          >     if (identical(target, current)) {
>          >         TRUE
>          >     } else {
>          >         msg <- NULL # TODO: check attributes
>          >         # deal with emptyenv now since parent.env(emptyenv())
>     gives error
>          >         # and topenv(emptyenv()) gives GlobalEnv
>          >         eTarget <- identical(target, emptyenv()) ||
>          > identical(target,topenv(target))
>          >         eCurrent <- identical(current, emptyenv()) ||
>          > identical(current,topenv(current))
>          >         if (eTarget || eCurrent) {
>          >             msg <- c(msg, paste("target is", format(target),
>     "and current
>          > is", format(current)))
>          >         } else {
>          >             thisComparison <- .all.equal.environment(target,
>     current, ...)
>          >             if (!isTRUE(thisComparison)) {
>          >                 msg <- c(msg, thisComparison)
>          >             }
>          >             parentComparison <- Recall(parent.env(target),
>          > parent.env(current), ...)
>          >             if (!isTRUE(parentComparison)) {
>          >                 msg <- c(msg, paste("<parent.env>",
>     parentComparison))
>          >             }
>          >         }
>          >         if (is.null(msg) || isTRUE(msg)) TRUE else msg
>          >     }
>          > }
>
>     Thank you, Duncan and Bill (and Kevin for bringing up the
>     topic).
>
>     I agree  all.equal() should work better with functions,
>
>     and I think probably it would make sense to define  all.equal.function()
>     rather than put this into all.equal.default()
>
>     However, it's not quite clear if it is always desirable to check the
>     environments as well notably as that *is* done recursively.
>
>     Bill, I'm sure you've noticed that we did write  all.equal.environment()
>     to work recursively... Actually, I had worked quite a bit at
>     that, too long ago to remember details, but the relevant svn log
>     entry is
>     ------------------------------------------------------------------------
>     r66640 | maechler | 2014-09-18 22:10:20 +0200 (Thu, 18 Sep 2014) | 1
>     line
>
>     more sophisticated all.equal.environment(): no longer "simple"
>     infinite recursions
>     ------------------------------------------------------------------------
>
>     Are you sure that code with the internal recursive do1()
>     function should/could not be amended where needed?
>
>     Martin
>
>          > On Mon, Nov 30, 2020 at 10:42 AM Duncan Murdoch
>     <[hidden email] <mailto:[hidden email]>>
>          > wrote:
>          >
>          > > On 30/11/2020 1:05 p.m., Kevin Van Horn via R-devel wrote:
>          > > > Consider the following code:
>          > > >
>          > > >      f <- function(x)function(y){x+y}
>          > > >      all.equal(f(5), f(0))
>          > > >
>          > > > This returns TRUE, when it should return FALSE; I think
>     it’s hard to
>          > > make the case that f(5) and f(0) are “approximately equal”
>     in any
>          > > meaningful sense. Digging into the code for all.equal(), I
>     see that
>          > > all.equal(f(5), f(0)) results in a call to
>     all.equal.language(f(5), f(0)),
>          > > which only compares the function texts for equality.
>          > > >
>          > > > If it is decided to leave this behavior as-is, then at
>     least it should
>          > > be documented. Currently I cannot find any documentation
>     for all.equal
>          > > applied to functions.
>          > >
>          > > Clearly it should also compare the environments of the two
>     functions,
>          > > then it would see a difference:
>          > >
>          > >  > all.equal(environment(f(5)), environment(f(0)))
>          > > [1] "Component “x”: Mean relative difference: 1"
>          > >
>          > > Changing the first few lines from
>          > >
>          > >      if (is.language(target) || is.function(target))
>          > >          return(all.equal.language(target, current, ...))
>          > >
>          > > to
>          > >
>          > >      if (is.function(target)) {
>          > >          msg <- all.equal.language(target, current, ...)
>          > >          if (isTRUE(msg)) {
>          > >              msg <- all.equal.environment(environment(target),
>          > > environment(current), ...)
>          > >              if (is.character(msg))
>          > >                msg <- paste("Environments:", msg)
>          > >          }
>          > >          return(msg)
>          > >      }
>          > >      if (is.language(target))
>          > >          return(all.equal.language(target, current, ...))
>          > >
>          > > would fix it.
>          > >
>          > > Duncan Murdoch
>          > >
>          > > ______________________________________________
>          > > [hidden email] <mailto:[hidden email]>
>     mailing list
>          > > https://stat.ethz.ch/mailman/listinfo/r-devel
>     <https://stat.ethz.ch/mailman/listinfo/r-devel>
>

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Reply | Threaded
Open this post in threaded view
|

Re: all.equal applied to function closures

aBBy Spurdle, ⍺XY
In reply to this post by Martin Maechler
> Bill, I'm sure you've noticed that we did write  all.equal.environment()
> to work recursively... Actually, I had worked quite a bit at
> that, too long ago to remember details, but the relevant svn log
> entry is
> ------------------------------------------------------------------------
> r66640 | maechler | 2014-09-18 22:10:20 +0200 (Thu, 18 Sep 2014) | 1 line
>
> more sophisticated all.equal.environment(): no longer "simple" infinite recursions
> ------------------------------------------------------------------------

I haven't checked the above reference.
But I would like to note the following behaviour:

    #e group
    e = new.env ()
    e1 = new.env ()
    e$e = e1
    e1$e = e

    #f group
    f = new.env ()
    f1 = new.env ()
    f2 = new.env ()
    f$e = f1
    f1$e = f2
    f2$e = f

    all.equal (e, f)

I tried a number of examples with circular references.
All worked correctly, except for "identical" environments nested an
unequal number of times.

I suspect there may be other special cases.

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Reply | Threaded
Open this post in threaded view
|

Re: all.equal applied to function closures

Martin Maechler
In reply to this post by Martin Maechler
>>>>> Martin Maechler
>>>>>     on Tue, 1 Dec 2020 10:31:36 +0100 writes:

>>>>> Bill Dunlap
>>>>>     on Mon, 30 Nov 2020 13:41:54 -0800 writes:

    >> To make the comparison more complete,
    >> all.equal.environment could compare the parents of the
    >> target and current environments.  That would have to be
    >> recursive but could stop at the first 'top level
    >> environment' (the global, empty, or a package-related
    >> environment generally) and use identical there.  E.g.,

    >> > f1 <- function(x) (function(){ expx <- exp(x) ; function(y) y + expx})()
    >> > all.equal(f1(2), f1(3))
    >> [1] "Environments: Component “expx”: Mean relative difference: 1.718282"
    >>
    >> [2] "Environments: <parent.env> Component “x”: Mean relative difference:
    >> 0.5"

    >> This is from the following, where I avoided putting the existing
    >> non-recursive all.equal.environment into the body of this one.

    [[.........]]

    > Thank you, Duncan and Bill (and Kevin for bringing up the
    > topic).

    > I agree  all.equal() should work better with functions,

    > and I think probably it would make sense to define  all.equal.function()
    > rather than put this into all.equal.default()

    > However, it's not quite clear if it is always desirable to check the
    > environments as well notably as that *is* done recursively.

A small  "work in progress" update (because I've been advancing slowly only):

I'm currently testing 'make check-all' after

1) adding all.equal.function() method,
2) not changing  all.equal.environment() yet
3) but adapting all.equal.default() to give a deprecating warning
   when called with a function

all.equal.function <- function(target, current, check.environments = TRUE, ...)
{
    msg <- all.equal.language(target, current, ...)
    if(check.environments) {
        ## pre-check identical(), for speed *and* against infinite recursion:
        ee <- identical(environment(target), environment(current), ignore.environment=FALSE)
        if(!ee)
            ee <- all.equal.environment(environment(target), environment(current), ...)
        if(isTRUE(msg))
            ee
        else
            c(msg, if(!isTRUE(ee)) ee)
    } else
        msg
}

It's amazing that this breaks our own checks in several places,
which I'm chasing slowly (being busy with teaching related
duties, etc).
I did have the correct gut feeling to have 'check.environments'
being an optional argument, because indeed there are cases (in
our own regression tests) where we now needed to change the
checks to use   check.environments=FALSE .

My plan is to finish the   all.equal.function()  functionality /
modify "base R" coded (or, for now, rather the testing code)
where needed, and then *commit* that to R-devel.

After that we should come back to improve or even re-write
all.equal.environment()  when needed.

Martin

    > Bill, I'm sure you've noticed that we did write  all.equal.environment()
    > to work recursively... Actually, I had worked quite a bit at
    > that, too long ago to remember details, but the relevant svn log
    > entry is
    > ------------------------------------------------------------------------
    > r66640 | maechler | 2014-09-18 22:10:20 +0200 (Thu, 18 Sep 2014) | 1 line

    > more sophisticated all.equal.environment(): no longer "simple" infinite recursions
    > ------------------------------------------------------------------------

    > Are you sure that code with the internal recursive do1()
    > function should/could not be amended where needed?

    > Martin

    >> On Mon, Nov 30, 2020 at 10:42 AM Duncan Murdoch <[hidden email]>
    >> wrote:
    >>
    >> > On 30/11/2020 1:05 p.m., Kevin Van Horn via R-devel wrote:
    >> > > Consider the following code:
    >> > >
    >> > >      f <- function(x)function(y){x+y}
    >> > >      all.equal(f(5), f(0))
    >> > >
    >> > > This returns TRUE, when it should return FALSE; I think it’s hard to
    >> > make the case that f(5) and f(0) are “approximately equal” in any
    >> > meaningful sense. Digging into the code for all.equal(), I see that
    >> > all.equal(f(5), f(0)) results in a call to all.equal.language(f(5), f(0)),
    >> > which only compares the function texts for equality.
    >> > >
    >> > > If it is decided to leave this behavior as-is, then at least it should
    >> > be documented. Currently I cannot find any documentation for all.equal
    >> > applied to functions.
    >> >
    >> > Clearly it should also compare the environments of the two functions,
    >> > then it would see a difference:
    >> >
    >> >  > all.equal(environment(f(5)), environment(f(0)))
    >> > [1] "Component “x”: Mean relative difference: 1"
    >> >
    >> > Changing the first few lines from
    >> >
    >> >      if (is.language(target) || is.function(target))
    >> >          return(all.equal.language(target, current, ...))
    >> >
    >> > to
    >> >
    >> >      if (is.function(target)) {
    >> >          msg <- all.equal.language(target, current, ...)
    >> >          if (isTRUE(msg)) {
    >> >              msg <- all.equal.environment(environment(target),
    >> > environment(current), ...)
    >> >              if (is.character(msg))
    >> >                msg <- paste("Environments:", msg)
    >> >          }
    >> >          return(msg)
    >> >      }
    >> >      if (is.language(target))
    >> >          return(all.equal.language(target, current, ...))
    >> >
    >> > would fix it.
    >> >
    >> > Duncan Murdoch
    >> >
    >> > ______________________________________________
    >> > [hidden email] mailing list
    >> > https://stat.ethz.ch/mailman/listinfo/r-devel

    > ______________________________________________
    > [hidden email] mailing list
    > https://stat.ethz.ch/mailman/listinfo/r-devel

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Reply | Threaded
Open this post in threaded view
|

all.equal() applied to function closures -- now committed

Martin Maechler
>>>>> Martin Maechler
>>>>>     on Thu, 3 Dec 2020 18:29:58 +0100 writes:

>>>>> Martin Maechler
>>>>>     on Tue, 1 Dec 2020 10:31:36 +0100 writes:

>>>>> Bill Dunlap
>>>>>     on Mon, 30 Nov 2020 13:41:54 -0800 writes:

    >>> To make the comparison more complete,
    >>> all.equal.environment could compare the parents of the
    >>> target and current environments.  That would have to be
    >>> recursive but could stop at the first 'top level
    >>> environment' (the global, empty, or a package-related
    >>> environment generally) and use identical there.  E.g.,

    >>> > f1 <- function(x) (function(){ expx <- exp(x) ; function(y) y + expx})()
    >>> > all.equal(f1(2), f1(3))
    >>> [1] "Environments: Component “expx”: Mean relative difference: 1.718282"
    >>>
    >>> [2] "Environments: <parent.env> Component “x”: Mean relative difference:
    >>> 0.5"

    >>> This is from the following, where I avoided putting the existing
    >>> non-recursive all.equal.environment into the body of this one.

    > [[.........]]

    >> Thank you, Duncan and Bill (and Kevin for bringing up the
    >> topic).

    >> I agree  all.equal() should work better with functions,

    >> and I think probably it would make sense to define  all.equal.function()
    >> rather than put this into all.equal.default()

    >> However, it's not quite clear if it is always desirable to check the
    >> environments as well notably as that *is* done recursively.

    > A small  "work in progress" update (because I've been advancing slowly only):

    > I'm currently testing 'make check-all' after

    > 1) adding all.equal.function() method,
    > 2) not changing  all.equal.environment() yet
    > 3) but adapting all.equal.default() to give a deprecating warning
    > when called with a function

    > all.equal.function <- function(target, current, check.environments = TRUE, ...)
    > {
    [........]
    > }

    > It's amazing that this breaks our own checks in several places,
    > which I'm chasing slowly (being busy with teaching related
    > duties, etc).
    > I did have the correct gut feeling to have 'check.environments'
    > being an optional argument, because indeed there are cases (in
    > our own regression tests) where we now needed to change the
    > checks to use   check.environments=FALSE .

    > My plan is to finish the   all.equal.function()  functionality /
    > modify "base R" coded (or, for now, rather the testing code)
    > where needed, and then *commit* that to R-devel.

I've done that now:
------------------------------------------------------------------------
r79555 | maechler | 2020-12-04 12:13:06 +0100 (Fri, 04 Dec 2020) | 1 line
Changed paths:
   M doc/NEWS.Rd
   M src/library/base/R/all.equal.R
   M src/library/base/R/zzz.R
   M src/library/base/man/all.equal.Rd
   M src/library/base/man/dput.Rd
   M src/library/stats/tests/nls.R
   M src/library/stats/tests/nls.Rout.save
   M tests/eval-etc-2.Rout.save
   M tests/eval-etc.Rout.save
   M tests/eval-fns.R
   M tests/reg-tests-1d.R

new all.equal.function() checks environment(.)
------------------------------------------------------------------------

Note the seven extra files needed to be changed

  src/library/base/man/dput.Rd
  src/library/stats/tests/nls.{R,Rout.save}
  tests/eval-{etc.Rout.save,fns.Rtests/eval-etc{,-2}.Rout.save}

where in all cases I needed to add an explicit
', check.environments = FALSE' to an existing  stopifnot(all.equal(..))
check.

Consequently, I expect some  "carnage" in checks of existing  CRAN / BioC
(and github, etc but that does not count) packages.

The NEWS entry (in 'NEW FEATURES') mentions an important case :

    • all.equal(f, g) for functions now by default also compares their
      environment(.)s, notably via new all.equal method for class
      function.  Comparison of nls() fits, e.g., may now need
      all.equal(m1, m2, check.environments=FALSE).

So if you are a package maintainer observing that all.equal() --
or its corresponding 'testthat' obfuscation -- signals errors
"suddenly" (and only in R-devel), it's that you need to add
'check.environments=FALSE' there.

The good news is that this works also in existing (and previous)
versions of R :

  > all.equal(lm,lm, check.environments=FALSE)
  [1] TRUE
  >

thanks to the "..."  _feature_ of typically just silently swallowing
everything that's not understood.  (yes: _feature_ is tongue in cheek).

Martin


    > After that we should come back to improve or even re-write
    > all.equal.environment()  when needed.

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel