-
Notifications
You must be signed in to change notification settings - Fork 73
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
[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>
📝 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/. |
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.
@djeong20, 💯 All CI checkers are successfully verified. Thanks.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of switch statement why don't we use function overriding as like other mathematical operations?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
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.
From the perspective of clean code and maintainability, it looks much better.
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.
@djeong20, 💯 All CI checkers are successfully verified. Thanks.
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.
LGTM
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
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.
LGTM
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: