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

Can't set Tensor slice using indexing #521

Closed
lostmsu opened this issue Feb 17, 2022 · 7 comments · Fixed by #522
Closed

Can't set Tensor slice using indexing #521

lostmsu opened this issue Feb 17, 2022 · 7 comments · Fixed by #522

Comments

@lostmsu
Copy link
Contributor

lostmsu commented Feb 17, 2022

This fails:

using var t = tensor(new[] { 1, 2, 3, 4 }, rows: 2, columns: 2);
t[0] = tensor(new[] { 5, 6 }); // <- exception
Assert.Equal(5, t[0, 0].ToInt32());

Error happens here:

THSTensor_set1(Handle, i, value.ToScalar().Handle);

Because the value is not a scalar.

Workaround

t[0].copy_(tensor(new[] { 5, 6 }))`
@lostmsu
Copy link
Contributor Author

lostmsu commented Feb 17, 2022

Was there any reason why this ToScalar call had to be present? I just tried on my machine, and adding a simple

void THSTensor_set1t(const Tensor tensor, int64_t index, const Tensor value)

And replacing the call above with

THSTensor_set1t(Handle, i, value.Handle);

seems to be working fine.

@NiklasGustafsson
Copy link
Contributor

NiklasGustafsson commented Feb 17, 2022

@lostmsu -- yeah, I can't figure out why it would want the source to be a Scalar there. There's a reason to special-case for the index being an integer, but not for the value to be scalar, I think.

I think the same applies to set2-set6,

Put it in a PR, and we can get it into 0.96.1.

I also just noted that all the C# extern declarations of THSTensor_setN methods should be void-returning, not IntPtr.

@NiklasGustafsson
Copy link
Contributor

In the meantime, you should be able to rely on the TensorIndex versions of the indexer.

@NiklasGustafsson
Copy link
Contributor

BTW, I usually add at least one regression test to the 'TestTorchTensorBugs.cs' file when I have a small enough repro.

@lostmsu
Copy link
Contributor Author

lostmsu commented Feb 17, 2022

@NiklasGustafsson can I replace the existing definitions? Should I even be worried about having incompatible C++ signatures between 0.96.0 and 0.96.1?

@NiklasGustafsson
Copy link
Contributor

Until we get to 1.0, we are unapologetically making breaking changes and there's no guarantee of binary compatibility.

Breaking changes need to be highlighted in the RELEASENOTES, but otherwise, let's just get things in order. The existing code is clearly incorrect.

@NiklasGustafsson
Copy link
Contributor

The parameter groups support will also be a breaking change because optimizers won't have a scalar 'LearningRate' property (those go with the parameter groups).

lostmsu added a commit to losttech/TorchSharp that referenced this issue Feb 18, 2022
Tensor int-based indexers forced `AsScalar()` call on assigned tensors, which prevented assigning tensor slices.

fixes dotnet#521
lostmsu added a commit to losttech/TorchSharp that referenced this issue Feb 18, 2022
Tensor int-based indexers forced `AsScalar()` call on assigned tensors, which prevented assigning tensor slices.

fixes dotnet#521
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 a pull request may close this issue.

2 participants