Skip to content

grad property - #1293

Closed
yueyinqiu wants to merge 11 commits into
dotnet:mainfrom
yueyinqiu:grad
Closed

yueyinqiu wants to merge 11 commits into
dotnet:mainfrom
yueyinqiu:grad

Conversation

@yueyinqiu

Copy link
Copy Markdown
Contributor

close #1291

@yueyinqiu

yueyinqiu commented Apr 26, 2024 •

Copy link
Copy Markdown
Contributor Author

By the way how about the circular reference?

using TorchSharp;

for (; ; )
{
    var t = torch.zeros(1000);
    t.grad = t;
    // t.set_grad(t);
}

PyTorch does not allow that:

image

However if the trick is played on two tensors, it's ok:

import torch

t = torch.zeros(1000)
g = torch.zeros(1000)
t.grad = g
g.grad = t

I have no idea how to deal with that... Just keep it leaking?

@yueyinqiu
yueyinqiu marked this pull request as ready for review April 26, 2024 06:24
return new Tensor(res);
}
set {
NativeMethods.THSTensor_set_grad(Handle, value?.Handle ?? IntPtr.Zero);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you remove the detach from dispose scope?
You also removed the MoveToOtherDisposeScope which was listed above in the to function.

@yueyinqiu yueyinqiu Apr 26, 2024 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that I have misunderstood something... I have reverted them.

@yueyinqiu yueyinqiu Apr 29, 2024 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shaltielshmid

I have reviewed it again and I'm now more confident about the removal. Could you please check my comment here and the new commit about this?

@yueyinqiu
yueyinqiu marked this pull request as draft April 26, 2024 14:36
@yueyinqiu
yueyinqiu marked this pull request as ready for review April 26, 2024 14:48
@yueyinqiu
yueyinqiu marked this pull request as draft April 26, 2024 14:50
@yueyinqiu

Copy link
Copy Markdown
Contributor Author

Perhaps I shall create a new pull request for this...

@yueyinqiu yueyinqiu closed this Apr 26, 2024
@yueyinqiu

Copy link
Copy Markdown
Contributor Author

#1299

@yueyinqiu yueyinqiu mentioned this pull request Apr 27, 2024
@yueyinqiu
yueyinqiu deleted the grad branch May 1, 2024 13:57
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.

should Tensor.grad be a property instead of a method?

2 participants