search

mlverse / torch

R Interface to Torch
https://torch.mlverse.org
Other
483 stars 66 forks source link

feat: comparison of torch tensors #1139

Closed sebffischer closed 4 months ago

sebffischer commented 4 months ago

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.

sebffischer commented 4 months ago

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

dfalbel commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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

dfalbel commented 4 months ago

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 commented 4 months ago

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

dfalbel commented 4 months ago

Thank you @sebffischer !