Add new TensorAxes type, remove TensorOptions::operator==()#27572
Add new TensorAxes type, remove TensorOptions::operator==()#27572andreaskoepf wants to merge 3 commits intopytorch:masterfrom
Conversation
As discussed in pytorch#25478 the TensorOptions::operator==() and the corresponding operator!=() had non-obvious comparison semantics. A new TensorAxes type is added that always holds a full axes specification consisting of {d_type, device, layout and is_variable} flag. TensorAxes objects can be safely compared. Fixes: pytorch#25478
3b69f8e to
6129fda
Compare
|
(committed while on the go, there seems to be a compile error, will fix later in the hotel, sry). |
468a6ee to
3fefbae
Compare
3fefbae to
5418404
Compare
|
This PR is now ready for review. A question that came up is what should happen to' Tensor::options()'. I first added a I see the following simple solutions: a) let options() return For the time being I resorted to c) - having both |
| /// `Tensor`. Additionally it indicates whether a `Tensor` is an autograd | ||
| /// variable. | ||
| /// | ||
| /// The class is in general immutable but allows to change the representation |
There was a problem hiding this comment.
but allows to change doesn't work grammatically. I think it should be something like but the representation as a whole can be changed via ...
There was a problem hiding this comment.
@resistor thanks a lot for the comment! As non native speaker the docs part is always 10x harder for me than writing pure code ;-). I highly appreciate any help/corrections on that side! :-)
| std::string TensorAxes::toString() const { | ||
| std::stringstream ss; | ||
| ss << *this; | ||
| return ss.str(); |
There was a problem hiding this comment.
It may not matter much, but I think this is introducing an extra string copy. The stringstream will have to copy from its internal storage into a temporary string. It would be more efficient to construct a string first, pass it to the by-reference constructor to stringstream, and then return it at the end.
|
A new design for |
As discussed in #25478 the TensorOptions::operator==() and the
corresponding operator!=() had non-obvious comparison semantics.
A new TensorAxes type is added that always holds a full axes
specification consisting of {d_type, device, layout and
is_variable} flag. TensorAxes objects can be safely compared.
Fixes: #25478