Skip to content

all.equal() problems #2751

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
krlmlr opened this issue May 6, 2017 · 10 comments · Fixed by #4657
Closed

all.equal() problems #2751

krlmlr opened this issue May 6, 2017 · 10 comments · Fixed by #4657
Labels
breaking change ☠️ API change likely to affect existing code feature a feature request or enhancement wip work in progress

Comments

@krlmlr
Copy link
Member

krlmlr commented May 6, 2017

Would it be too much of a change to get rid of this override altogether? I propose to emit a warning that all_equal() should be used on tibbles in v0.6, and then remove the specialization in v0.7.

CC @hadley @jennybc.

@hadley
Copy link
Member

hadley commented May 8, 2017

I think that's ok, although I feel like there's probably quite a few places where I rely on test_equal() working. It's probably better to be explicit though.

An alternative fix would be to write our own all.equal package and systematically work through all of the issues. I'm not sure that's worth the investment now, although it's probably worth it in the long run.

@hadley hadley added feature a feature request or enhancement data frame labels May 15, 2017
@krlmlr
Copy link
Member Author

krlmlr commented May 15, 2017

@hadley: Are we including this in dplyr 0.6.0?

@hadley
Copy link
Member

hadley commented May 15, 2017

No, it'll have to wait until the next version.

@PaulHiemstra
Copy link

Is it clear when this fix will be included in dplyr?

topepo pushed a commit to tidymodels/recipes that referenced this issue Jan 10, 2018
@krlmlr krlmlr added the breaking change ☠️ API change likely to affect existing code label Apr 18, 2018
@mikmart
Copy link
Contributor

mikmart commented Aug 3, 2018

Are there any thoughts on when/if attribute checking might be implemented? I'm creating a tbl_df subclass that just adds an attribute, and was surprised when some tests that I expected to fail, didn't.

For now I'm using a (rather janky) workaround with a custom all.equal() method. But this still doesn't allow for simple testing that a lossless roundtrip is possible from tbl_df to the subclass and back.

@yutannihilation
Copy link
Member

In case this is useful, dplyr's all.equal.tbl_df() is also problematic for data.frame columns, which the bare all.equal() handles well.

library(tibble)

d1 <- tibble(id = 1:3, x = tibble(a = 1:3, y = 1:3))
d2 <- tibble(id = 1:3, x = tibble(a = 1:3, y = NA))

# works fine
all.equal(d1, d1)
#> [1] TRUE
all.equal(d1, d2)
#> [1] "Component \"x\": Component \"y\": Modes: numeric, logical"              
#> [2] "Component \"x\": Component \"y\": target is numeric, current is logical"

library(dplyr, warn.conflicts = FALSE)

# broken
all.equal(d1, d1)
#> Error: Can't join on 'x' x 'x' because of incompatible types (tbl_df/tbl/data.frame / tbl_df/tbl/data.frame)
all.equal(d1, d2)
#> Error: Can't join on 'x' x 'x' because of incompatible types (tbl_df/tbl/data.frame / tbl_df/tbl/data.frame)

Created on 2019-02-04 by the reprex package (v0.2.1.9000)

@espinielli
Copy link
Contributor

I have been beaten by this (and went crazy 😓) in the last few days.
Finally I resolved to use the conversion to data.frame to make testthat::expect_equal() work 😆:

expect_equal(as.data.frame(object), as.data.frame(expected))

HTH

@tvatter
Copy link

tvatter commented Oct 9, 2019

Quick question: the tolerance issue mentioned as the first bullet point of the first message hasn't been resolved or am I missing something?

It would really be nice to be able to account for small differences like all.equal(df, df + 1e-9) via the usual tolerance argument.

@hadley
Copy link
Member

hadley commented Dec 10, 2019

I think we should plan to rip out the existing all.equal.tbl_df method as it's clearly not correct. This will require updating a lot of tests, and will probably break some downstream packages, but I think it's worth it.

@hadley hadley removed the data frame label Dec 11, 2019
hadley added a commit that referenced this issue Dec 12, 2019
And update newly failing tests.

Fixes #2751
@hadley hadley added the wip work in progress label Dec 12, 2019
hadley added a commit that referenced this issue Dec 13, 2019
And update newly failing tests.

Fixes #2751
@lock
Copy link

lock bot commented Jun 24, 2020

This old issue has been automatically locked. If you believe you have found a related problem, please file a new issue (with reprex) and link to this issue. https://reprex.tidyverse.org/

@lock lock bot locked and limited conversation to collaborators Jun 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
breaking change ☠️ API change likely to affect existing code feature a feature request or enhancement wip work in progress
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants