R devel - Apr 2019 - all.equal failure and [.terms

>>>>> Duncan Murdoch 
>>>>>     on Fri, 5 Apr 2019 11:12:48 -0400 writes:
    > On 05/04/2019 10:46 a.m., Therneau, Terry M., Ph.D. wrote:
    >> 
    >> 
    >> On 4/5/19 9:39 AM, Duncan Murdoch wrote:
    >>> On 05/04/2019 10:19 a.m., Therneau, Terry M., Ph.D. wrote:
    >>>> Duncan,
    >>>> ?? I should have included it in my original note, but
    >>>> 
    >>>> ??? ? all.equal(unclass(t0x), unclass(t1x))
    >>>> 
    >>>> returns TRUE as well.? I had tried that as well. ? But a
further look at
    >>>> all.equal.default shows the following line right near the
top:
    >>>> ???? if (is.language(target) || is.function(target))
    >>>> ???????? return(all.equal.language(target, current, ...))
    >>>> 
    >>>> and that path explicitly ignores attributes.
    >>> 
    >>> Which R version are you using?? I see deparse(target) and
deparse(current) in
    >>> all.equal.language(), and those should not be ignoring
attributes according to the
    >>> documentation.

But the problem is that indeed  "of course"  all.equal.formula()
and not all.equal.language() is called for the terms since as
you yourself remarked, their class is  c("terms",
"formula"),

and so what Terry reported is indeed correct *and* a bug
and in "all versions" of R (I did not look far back, but these things
haven't changed much).

The cleanest would probably be to define an  all.equal.terms()
method, as I think there may be more code relying on the
behavior of  all.equal.formula() to only look at the formulas
themselves and not their attributes...
but you (Duncan) and others may have a different opinion.

Martin Maechler
ETH Zurich and R Core Team

>>>>> Martin Maechler 
>>>>>     on Fri, 5 Apr 2019 17:33:54 +0200 writes:
>>>>> Duncan Murdoch 
>>>>>     on Fri, 5 Apr 2019 11:12:48 -0400 writes:
    >> On 05/04/2019 10:46 a.m., Therneau, Terry M., Ph.D. wrote:
    >>> 
    >>> 
    >>> On 4/5/19 9:39 AM, Duncan Murdoch wrote:
    >>>> On 05/04/2019 10:19 a.m., Therneau, Terry M., Ph.D. wrote:
    >>>>> Duncan,
    >>>>> ?? I should have included it in my original note, but
    >>>>> 
    >>>>> ??? ? all.equal(unclass(t0x), unclass(t1x))
    >>>>> 
    >>>>> returns TRUE as well.? I had tried that as well. ? But
a further look at
    >>>>> all.equal.default shows the following line right near
the top:
    >>>>> ???? if (is.language(target) || is.function(target))
    >>>>> ???????? return(all.equal.language(target, current,
...))
    >>>>> 
    >>>>> and that path explicitly ignores attributes.
    >>>> 
    >>>> Which R version are you using?? I see deparse(target) and
deparse(current) in
    >>>> all.equal.language(), and those should not be ignoring
attributes according to the
    >>>> documentation.

    > But the problem is that indeed  "of course" 
all.equal.formula()
    > and not all.equal.language() is called for the terms since as
    > you yourself remarked, their class is  c("terms",
"formula"),

    > and so what Terry reported is indeed correct *and* a bug
    > and in "all versions" of R (I did not look far back, but
these things
    > haven't changed much).

    > The cleanest would probably be to define an  all.equal.terms()
    > method, as I think there may be more code relying on the
    > behavior of  all.equal.formula() to only look at the formulas
    > themselves and not their attributes...
    > but you (Duncan) and others may have a different opinion.

and I do agree with Duncan even more now that indeed it's very
unsatisfactory that deparse() {and dput(), dump() ..} of a terms
object would only reproduce the formula and nothing else;
and yes that's all in the C code:
 --> src/main/deparse.c
    --> in function deparse2buff()
       -->  inside the (350 lines large)  'case LANGSXP'.

Martin

On 05/04/2019 11:33 a.m., Martin Maechler wrote:
>>>>>> Duncan Murdoch
>>>>>>      on Fri, 5 Apr 2019 11:12:48 -0400 writes:
> 
>      > On 05/04/2019 10:46 a.m., Therneau, Terry M., Ph.D. wrote:
>      >>
>      >>
>      >> On 4/5/19 9:39 AM, Duncan Murdoch wrote:
>      >>> On 05/04/2019 10:19 a.m., Therneau, Terry M., Ph.D.
wrote:
>      >>>> Duncan,
>      >>>> ?? I should have included it in my original note, but
>      >>>>
>      >>>> ??? ? all.equal(unclass(t0x), unclass(t1x))
>      >>>>
>      >>>> returns TRUE as well.? I had tried that as well. ?
But a further look at
>      >>>> all.equal.default shows the following line right near
the top:
>      >>>> ???? if (is.language(target) || is.function(target))
>      >>>> ???????? return(all.equal.language(target, current,
...))
>      >>>>
>      >>>> and that path explicitly ignores attributes.
>      >>>
>      >>> Which R version are you using?? I see deparse(target) and
deparse(current) in
>      >>> all.equal.language(), and those should not be ignoring
attributes according to the
>      >>> documentation.
> 
> But the problem is that indeed  "of course"  all.equal.formula()
> and not all.equal.language() is called for the terms since as
> you yourself remarked, their class is  c("terms",
"formula"),
> 
> and so what Terry reported is indeed correct *and* a bug
> and in "all versions" of R (I did not look far back, but these
things
> haven't changed much).
> 
> The cleanest would probably be to define an  all.equal.terms()
> method, as I think there may be more code relying on the
> behavior of  all.equal.formula() to only look at the formulas
> themselves and not their attributes...
> but you (Duncan) and others may have a different opinion.
I don't know if that would be easy -- it seems to me there is a bug in 
deparse(), which won't show attributes on language objects even if you 
ask it to:

# This is fine:
deparse(structure(1, attrib=2))
# [1] "structure(1, attrib = 2)"

# This doesn't show the attributes
deparse(structure(quote(f(1)), attrib=2))
# [1] "f(1)"

But as you mention, if this isn't a new bug fixing it will likely cause 
problems for people who assume it is intentional...

Duncan

The all.equal was a side issue for me; I don't have strong opinions one way
or the other.?
You are welcome to leave me out of the loop on that.? (Or leave me on the cc,
whatever is
easiest).

 ?I will update the survival package once the [.terms issues are addressed.

 ?One debatable issues is the choice of change vs document for the offset()
issue.? With
my proposed fix or without it, offsets are completely ignored by [.terms and
dropterms.?
So with a formula of

 ?? z <- terms(y~ x1 + offset(x2) + x3)

the 2 in drop.terms(z, 2) or z[-2] refers to x3, and the result will drop both
the offset
and x3.? For the use cases that I can think of the two functions are used at the
'build
the X matrix' stage, offsets have already been accounted for, and the
present behavior is
fine.? My vote would be to document it with a few lines in the help file since
that is the
easiest.? Offsets don't count as a 'term' in the assign attribute
either so the current
behavior is consistent in that respect.

Terry T.


	[[alternative HTML version deleted]]