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

implicit cast from Tensor to TensorIndex #1296

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

yueyinqiu
Copy link
Contributor

@yueyinqiu yueyinqiu commented Apr 26, 2024

closes #1266


It includes some aggressive changes. Careful checks are required.

@yueyinqiu
Copy link
Contributor Author

yueyinqiu commented Apr 26, 2024

  • I have removed the implicit operator from TensorIndex to Tensor.
    • I don't understand why it should exist to just throw an exception... but the behavior is even tested in the unit tests.
  • I have removed the two indexers this[params Tensor[] indices] and this[params long[] indices] since tensors could now be implicitly cast to TensorIndex.
    • However if there is something else, say a user defined class A, can be implicitly cast to a tensor, which used to be able to be used as the indices, is now unable to be used because it won't be automatically cast twice. This is happening for int? in TorchVision/AutoAugment.cs.

There might be other potential risks since it is an implicit operator to be added.

@yueyinqiu yueyinqiu marked this pull request as ready for review April 26, 2024 13:28
@NiklasGustafsson
Copy link
Contributor

Hmmm. Not sure about this one. Please elaborate.

@yueyinqiu
Copy link
Contributor Author

1. add implicit operator TensorIndex(Tensor tensor) into TensorIndex

This is what we want.

2. remove implicit operator Tensor(TensorIndex value) of TensorIndex

This operator is just throwing an exception. I don't know why it should exist, but this is even tested in a unit test.

3. remove Tensor this[params Tensor[] indices] of Tensor

Well... Now I suppose it shouldn't be removed... We need an override for a array.

4. remove Tensor this[params Tensor[] indices] of Tensor

Similar to 3.

@NiklasGustafsson
Copy link
Contributor

This operator is just throwing an exception.

I added it to make sure that some non-C# language (who knows?) doesn't apply different rules for implicit conversions.

@yueyinqiu yueyinqiu marked this pull request as draft May 1, 2024 14:57
@yueyinqiu
Copy link
Contributor Author

This operator is just throwing an exception.

I added it to make sure that some non-C# language (who knows?) doesn't apply different rules for implicit conversions.

Sorry, I haven't catch it. So seems that we could remove this now? Or we shall still keep it?

@yueyinqiu
Copy link
Contributor Author

yueyinqiu commented May 1, 2024

Amazing... A unit test failed because of the removal of that exception-throwing operator...

It's this line here:

image

If we don't have the implicit operator TensorIndex -> Tensor, it does not know which indexer should be used (params Tensor[] or params TensorIndex[] are both possible choices)...

And when we remove it and add the new Tensor -> TensorIndex, it will choose the params Tensor[] indexer... But the tensor converted from (0, 2) is a float tensor, which cannot be used as indices...

That's a bit...

Well... Do we really need the implicit operator from a float tuple to a tensor? Actually I suggest to make them explicit. Or we could add (long, long) -> Tensor as well, and then the unit test would pass. (But this may break other things...)

So amazing...


A possible solution would be still to remove Tensor this[params Tensor[] indices]. And add Tensor this[Tensor[] indices] for arrays.

@NiklasGustafsson
Copy link
Contributor

Implicit operators are complicated to get right, and since they're invisible in the code that uses them, hard to debug. I actually had to ask Mads Torgersen (lead PM on C#) to help me get this right. The operator you are trying to remove was important to declare but should never be called, as I remember it.

I'd rather we left these operators alone.

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.

Ranges implementation
2 participants