[Tensor] Update tensorbase for efficient creation of new tensor class. #2687
Conversation
This PR updates the TensorBase class to make mathematical operations that are not required to create a new tensor class. This change allows developers to easily create new classes without implementing math operations. Note that these functions should be implemented to utilize tensor operations fully. **Changes proposed in this PR:** - Change math operation function from pure virtual function to virtual function - Add a private function to get the data type as a string **Self-evaluation:** 1. Build test: [X]Passed [ ]Failed [ ]Skipped 2. Run test: [X]Passed [ ]Failed [ ]Skipped Signed-off-by: Donghyeon Jeong <dhyeon.jeong@samsung.com>
djeong20
requested review from
myungjoo,
jijoongmoon,
again4you,
jaeyun-jung,
leemgs,
wooksong,
helloahn,
kparichay,
gichan-jang,
anyj0527,
zhoonit,
lhs8928,
songgot,
jihochu,
DonghakPark,
SeoHyungjun,
baek2sm,
skykongkong8,
EunjuYang and
a team
as code owners
|
📝 TAOS-CI Version: 1.5.20200925. Thank you for submitting PR #2687. Please a submit 1commit/1PR (one commit per one PR) policy to get comments quickly from reviewers. Your PR must pass all verificiation processes of cibot before starting a review process from reviewers. If you are new member to join this project, please read manuals in documentation folder and wiki page. In order to monitor a progress status of your PR in more detail, visit http://ci.nnstreamer.ai/. |
nntrainer/tensor/tensor_base.h
Outdated
| * @brief Get the Data Type String object | ||
| * @return std::string of tensor data type | ||
| */ | ||
| std::string getStringDataType() const { |
There was a problem hiding this comment.
Instead of switch statement why don't we use function overriding as like other mathematical operations?
There was a problem hiding this comment.
Suggested code would be as follows.
class TensorBase {
virtual std::string getStringDataType() const {
return "Undefined type";
}
}
class FloatTensor : public TensorBase {
std::string getStringDataType() const override {
return "FP32";
}
}
class HalfTensor : public TensorBase {
std::string getStringDataType() const override {
return "FP16";
}
}
...
There was a problem hiding this comment.
Good point! This could definitely make a better code structure.
This patch updates the getStringDataType function structure to utilize method overriding. **Self-evaluation:** 1. Build test: [X]Passed [ ]Failed [ ]Skipped 2. Run test: [X]Passed [ ]Failed [ ]Skipped Signed-off-by: Donghyeon Jeong <dhyeon.jeong@samsung.com>
| Tensor &TensorBase::multiply(float const &value, Tensor &output) const { | ||
| throw std::invalid_argument( | ||
| "Tensor::multiply() is currently not supported in tensor data type " + | ||
| getStringDataType()); | ||
| return output; | ||
| } | ||
|
|
There was a problem hiding this comment.
I don't really get it.
How can I implement new math-related code on top of this?
Could you share some guidelines?
There was a problem hiding this comment.
As I understood, if there is no corresponding implementation to virtual functions implemented on tensor_base.h, would it fall to error msgs.. is that what you meant?
There was a problem hiding this comment.
not really. If no overridden function exists in the derived class, it will throw an error.
On top of that, you must implement any mathematical tensor operations in the derived class (e.g., FloatTensor).
This PR updates the TensorBase class to make mathematical operations that are not required to create a new tensor class.
This change allows developers to easily create new classes without implementing math operations.
Note that these functions should be implemented to utilize tensor operations fully.
Changes proposed in this PR:
Self-evaluation: