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

Discussion: methods vs. function in R and Python #83

Closed
skeydan opened this issue Jun 22, 2020 · 4 comments
Closed

Discussion: methods vs. function in R and Python #83

skeydan opened this issue Jun 22, 2020 · 4 comments
Labels
torch Issues in the torch namespace

Comments

@skeydan
Copy link
Collaborator

skeydan commented Jun 22, 2020

Currently, with tensor modification, our strategy seems to be:

  • R functions (torch_) only when a fresh object is returned
  • R methods (x$what()) only when the existing object is modified

e.g. tx$view() but torch_reshape

I assume this is why I don't see an x$clone()
(also I notice we have x$t_() but not x$t())

But, a person coming from Python might expect $ access to work independently of whether there are side effects or not (there is an x.clone(), etc.)

Not saying we have to follow this ... but partly we do: For example, there is x$mm as well as x$mm_ in R ...

What do you think?

@dfalbel
Copy link
Member

dfalbel commented Jun 22, 2020

IMHO we should closely follow python here. Ie if a method exists in Python it should also exist in R.

x$t() exists in R the side:

> x <- torch_randn(10, 5)
> x$t()
torch_tensor 
 1.5990 -1.2676  1.4600 -0.6393 -0.8593  1.0309  0.4477  1.0159  0.0463  0.0362
 2.1157  0.7238  0.0673  0.7179  0.1117 -1.0217 -2.2464  0.0867  1.2638 -0.7934
-1.2125 -1.0230 -0.0952 -1.1130  0.2201 -0.3773  0.5066 -0.8243 -0.3650 -0.2140
-0.2426  0.0644  0.2774 -0.9705  1.0606 -1.2880  0.8776 -0.4154  0.1012 -1.9847
-0.6819 -0.0620 -1.3663 -1.2802  0.7495 -0.3198 -1.4872 -0.9426  1.0952  0.2087
[ CPUFloatType{5,10} ]

clone is a special case, because it was not possible to add a clone method to an R6 class.
so we renamed it to copy. We should be able to call it clone again, since we are no longer using R6 classes for tensors.

@skeydan
Copy link
Collaborator Author

skeydan commented Jun 22, 2020

Oh, I completely agree! Perfect. Yes, then it'd be nice to have a clone as well!

Why I thought there was no x$t():

t$mm_(t$t())
Error: attempt to apply non-function

(but tbh I didn't investigate)

@dfalbel
Copy link
Member

dfalbel commented Jun 23, 2020

I think in this case, mm_ is the one that doesn't exist.

@dfalbel dfalbel added the torch Issues in the torch namespace label Jun 23, 2020
@skeydan
Copy link
Collaborator Author

skeydan commented Jun 23, 2020

oh, right - it really does not exist (in Python) :-)

Closing this and opening a separate one for clone :-)

@skeydan skeydan closed this as completed Jun 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
torch Issues in the torch namespace
Projects
None yet
Development

No branches or pull requests

2 participants