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

property grad #1299

Merged
merged 8 commits into from
May 7, 2024
Merged

property grad #1299

merged 8 commits into from
May 7, 2024

Conversation

yueyinqiu
Copy link
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
@yueyinqiu yueyinqiu marked this pull request as draft April 27, 2024 04:28
@yueyinqiu
Copy link
Contributor Author

fixed #1300

@yueyinqiu
Copy link
Contributor Author

yueyinqiu commented Apr 29, 2024

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
Contributor

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
Copy link
Contributor Author

yueyinqiu commented Apr 29, 2024

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
Copy link
Contributor Author

yueyinqiu commented May 1, 2024

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
Copy link
Contributor

NiklasGustafsson commented May 1, 2024 via email

@yueyinqiu
Copy link
Contributor Author

yueyinqiu commented May 1, 2024

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
Copy link
Contributor

NiklasGustafsson commented May 6, 2024

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
Copy link
Contributor Author

yueyinqiu commented May 6, 2024

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
Contributor

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

@NiklasGustafsson
Copy link
Contributor

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
Copy link
Contributor Author

yueyinqiu commented May 6, 2024

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
Copy link
Contributor Author

yueyinqiu commented May 7, 2024

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
Copy link
Contributor Author

yueyinqiu commented May 7, 2024

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
2 checks passed
@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