Skip to content
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

feat: comparison of torch tensors #1139

Merged
merged 8 commits into from
Mar 4, 2024

Conversation

sebffischer
Copy link
Collaborator

@sebffischer sebffischer commented Feb 19, 2024

I did not create a PR in waldo, because I think this method should be under full control of the torch package. I "hacked" it in, so waldo does not become a hard dependency here.

R/compare.R Outdated Show resolved Hide resolved
NEWS.md Outdated Show resolved Hide resolved
@sebffischer
Copy link
Collaborator Author

@dfalbel what is your opinion on this, do you want this in the torch package at all?

@dfalbel
Copy link
Member

dfalbel commented Feb 26, 2024

Yes, I think that's a useful feature! Do you mind checking why it fails on CI? We could also instead of registering onm package load, only register it when testing. Eg defining it in https://github.com/mlverse/torch/blob/main/tests/testthat/setup.R

@sebffischer
Copy link
Collaborator Author

It should be fixed now. I would prefer to add it during package so other people (including myself) can use it in their tests as well.

@sebffischer
Copy link
Collaborator Author

sebffischer commented Feb 27, 2024

I am a bit confused by the failing windows test and the centos failure does not seem to be caused by this PR.

@dfalbel
Copy link
Member

dfalbel commented Feb 28, 2024

Yes, it's failing on older R versions I think:

Error: Error: argument "path" is missing, with no default
Backtrace:
     x
  1. +-testthat::expect_failure(expect_equal(x, x$clone())) at test-compare.R:37:3
  2. | \-testthat::capture_expectation(expr)
  3. |   \-base::tryCatch(...)
  4. |     \-base (local) tryCatchList(expr, classes, parentenv, handlers)
  5. |       \-base (local) tryCatchOne(expr, names, parentenv, handlers[[1L]])
  6. |         \-base (local) doTryCatch(return(expr), name, parentenv, handler)
  7. \-testthat::expect_equal(x, x$clone())
  8.   \-testthat:::expect_waldo_equal("equal", act, exp, info, ..., tolerance = tolerance)
  9.     \-testthat:::waldo_compare(...)
 10.       \-waldo::compare(x, y, ..., x_arg = x_arg, y_arg = y_arg)
 11.         \-waldo:::compare_structure(x, y, paths = c(x_arg, y_arg), opts = opts)
 12.           +-waldo::compare_proxy(x)
 13.           \-torch:::compare_proxy.torch_tensor(x)

We could simply skip this tests if on R <= 4.0 if you want.

The centOS error is indeed not related to this PR, don't worry about it.

@sebffischer
Copy link
Collaborator Author

Thx for fixing the typo! I think we are good to merge here?

@dfalbel dfalbel merged commit 3a53954 into mlverse:main Mar 4, 2024
8 of 9 checks passed
@dfalbel
Copy link
Member

dfalbel commented Mar 4, 2024

Thank you @sebffischer !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants