-
Notifications
You must be signed in to change notification settings - Fork 176
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
base: main
Are you sure you want to change the base?
Conversation
There might be other potential risks since it is an implicit operator to be added. |
Hmmm. Not sure about this one. Please elaborate. |
1. add This is what we want. 2. remove 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 Well... Now I suppose it shouldn't be removed... We need an override for a array. 4. remove Similar to 3. |
This reverts commit 22021a2.
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? |
Amazing... A unit test failed because of the removal of that exception-throwing operator... It's this line here: If we don't have the implicit operator And when we remove it and add the new 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 So amazing... A possible solution would be still to remove |
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. |
closes #1266
It includes some aggressive changes. Careful checks are required.