Skip to content

property grad - #1299

Merged
NiklasGustafsson merged 8 commits into
dotnet:mainfrom
yueyinqiu:gradient
May 7, 2024
Merged

NiklasGustafsson merged 8 commits into
dotnet:mainfrom
yueyinqiu:gradient

Conversation

@yueyinqiu

Copy link
Copy Markdown
Contributor

close #1291

@yueyinqiu yueyinqiu mentioned this pull request Apr 26, 2024
@yueyinqiu
yueyinqiu marked this pull request as ready for review April 26, 2024 15:28
@yueyinqiu yueyinqiu mentioned this pull request Apr 26, 2024
Comment thread src/TorchSharp/NN/Module.cs Resolved
Comment thread src/TorchSharp/Tensor/Tensor.cs Resolved
@yueyinqiu
yueyinqiu marked this pull request as draft April 27, 2024 04:28
Comment thread src/TorchSharp/Tensor/Tensor.cs Resolved
@yueyinqiu

Copy link
Copy Markdown
Contributor Author

fixed #1300

@yueyinqiu

yueyinqiu commented Apr 29, 2024 •

Copy link
Copy Markdown
Contributor Author

Are there any suggestion about this behavior in PyTorch?

import torch

x = torch.zeros([], requires_grad=True)
y = torch.zeros([])

x.grad = y
print(y is x.grad)  # True

This is pretty strange that they are the same Python object. Perhaps PyTorch tensors are using a different technique instead of a simple wrapper of a pointer to a LibTorch tensor. (Maybe this is very common for Python. Actually I don't know how Python interacts with C++.)

The only way I can find is to also save the C# tensor when setting grad, and prefer to return it when getting.

@NiklasGustafsson

Copy link
Copy Markdown

Are there any suggestion about this behavior in PyTorch?

import torch

x = torch.zeros([], requires_grad=True)
y = torch.zeros([])

x.grad = y
print(y is x.grad)  # True

To maintain this behavior seems to be of pretty marginal value, right?

@yueyinqiu

yueyinqiu commented Apr 29, 2024 •

Copy link
Copy Markdown
Contributor Author

I suppose at least I will never rely on this.

I have made this pr ready for review/merging.

@yueyinqiu
yueyinqiu marked this pull request as ready for review April 29, 2024 15:21
@yueyinqiu

yueyinqiu commented May 1, 2024 •

Copy link
Copy Markdown
Contributor Author

The test seems to fail by chance.

Perhaps we need a seeded random for unit tests :)

Would it be a good idea to include pythonnet and check the results with PyTorch?

@NiklasGustafsson

NiklasGustafsson commented May 1, 2024 via email

Copy link
Copy Markdown

@yueyinqiu

yueyinqiu commented May 1, 2024 •

Copy link
Copy Markdown
Contributor Author

Yes, and I've found that torch's reproducibility system is not that reliable. I'm just considering something like C# System.Random, for our unit test alone.

@NiklasGustafsson

NiklasGustafsson commented May 6, 2024 •

Copy link
Copy Markdown

Hmmm. Tests are failing due to MNIST download. That has nothing to do with the PR, but I wonder if the source location has changed. If so, we may need to disable the test and then file an issue.

@yueyinqiu

yueyinqiu commented May 6, 2024 •

Copy link
Copy Markdown
Contributor Author

I suppose it's a temporary problem because train images, train labels and test images can all be accessed. (And test labels cannot.)

@NiklasGustafsson

Copy link
Copy Markdown

Let's hope. I'll try restarting the build a couple of times today.

@NiklasGustafsson

Copy link
Copy Markdown

It just keeps failing. Based on the stack trace, it's failing to download the first file, which is 'train-images-idx3-ubyte.gz' and when I click on the links above, it doesn't work. Maybe it's got something to do with where the requests come from? If the license allows it, we could add those files to the repo, instead. They're not big.

@yueyinqiu

yueyinqiu commented May 6, 2024 •

Copy link
Copy Markdown
Contributor Author

I suppose the best solution is to use mirrors.

In PyTorch there is a torchvision.datasets.MNIST.mirrors, whose default value is:

    mirrors = [
        "http://yann.lecun.com/exdb/mnist/",
        "https://ossci-datasets.s3.amazonaws.com/mnist/",
    ]

However torchvision.datasets.MNIST is not a class in TorchSharp. So

  • should we add that in Modules.MNIST? But this is inconsistent with PyTorch;
  • or make torchvision.datasets.MNIST a class? But then it can't be a method. Still inconsistent;
  • or maybe we could just simply add an extra parameter.

Edit:

I have checked that only mnist-like datasets have mirrors property. (Also, they must have such property, since they are inherited from MNIST.) Since it's not a common property of datasets, it seems that it's more like a inner property, which users are not encouraged to touch.

In that case, adding mirrors into Modules.MNIST is also acceptable. And this allows us to add more properties freely, like resources, training_file, test_file, classes and etc.

@yueyinqiu

yueyinqiu commented May 7, 2024 •

Copy link
Copy Markdown
Contributor Author

However current Modules.MNIST is internal. Perhaps what we should do currently is just to add the another default mirror.

I have created a PR here #1308

@yueyinqiu

yueyinqiu commented May 7, 2024 •

Copy link
Copy Markdown
Contributor Author

Maybe it's got something to do with where the requests come from?

Perhaps it is. I have found a relevant issue in pytorch pytorch/vision#7637

@NiklasGustafsson
NiklasGustafsson merged commit 8b14055 into dotnet:main May 7, 2024
@yueyinqiu
yueyinqiu deleted the gradient branch May 7, 2024 23:49
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?

3 participants