-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Misc Changes #7264
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
Misc Changes #7264
Conversation
…arp Range/Index implementation
|
@michaelgsharp Could you please help review this change? @ericstj could you please help reviewing the changes in the csproj files for a new added |
docs/samples/Microsoft.ML.AutoML.Samples/Microsoft.ML.AutoML.Samples.csproj
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dependency changes look ok, can you please review as well @michaelgsharp
| var padH = 32 - (image.Height % 32); | ||
| var transMidTensor = torch.zeros(1, 3, image.Height + padH, image.Width + padW, device: _parent.Device); | ||
| transMidTensor[.., .., ..image.Height, ..image.Width] = reMidTensor / 255.0; | ||
| transMidTensor[RangeUtil.ToTensorIndex(..), RangeUtil.ToTensorIndex(..), RangeUtil.ToTensorIndex(..image.Height), RangeUtil.ToTensorIndex(..image.Width)] = reMidTensor / 255.0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to make change this from ..?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
transMidTensor is accepting TensorIndex. And .. is a Range type which is defined in another library now. So, need a way to convert .. which is Range to TensorIndex. What can be done to make this syntax work again is to add implicit conversion inside the TensorIndex struct. @tannergooding may help advising about that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By the way, this syntax used to work before because we had our own implementation of Range which had implicit conversion to TensorIndex.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@michaelgsharp let me know if you want open issue track that?
The changes here include the following:
o1model labels in the Tiktoken tokenizerEncodedTokenwith theRangeand remove theTorchSharpRange/IndeximplementationSentencePieceBpeTokenizerto allow adding more models to it in the futureTokenizer.Decodemethod return a non-nullable stringEvery commit in the PR is representing one of the listed changes. Reviewing every commit separately will make it easier to have a clear understanding of the changes.