[ Tensor ] UInt Tensor : UInt8 / UInt16 / Uint32 @open sesame 09/23 16:50 #2735

EunjuYang · 2024-09-13T02:17:37Z

This commit is related to [ Tensor ] more types of Tensors are required. #2733
This commit implements template of UIntTensor based on the ShortTensor class.
Based on the template, this commit supports UInt8 / UInt16 / UInt32
Implement UIntTensor Template
Implement UInt8Tensor / UInt16Tensor / UInt32Tensor (ShortTensor is replaced)
Unit tests for UInt8 and UInt32 are added

Update 2024-09-23 14:54

Resolving NDK build error and reflecting some review comments.
This PR contains 11044d7 and 5916650
5916650 : removes some headers (float_tensor, char_tensor, uint_tensor.h), where their corresponding .cpp file imports tneosr.h. Reason of this change is
- In order to clarify the files relationship.
- uint_tensor.h should contain all implementations in one file, which is additional update in 11044d7.
11044d7 : defines template uint_tensor class based on ShortTensor. Since it is based on the template class, its implementation should be in one file (NDK build does not work with template class declaration)

Self evaluation:
Build test: [X]Passed [ ]Failed [ ]Skipped
Run test: [X]Passed [ ]Failed [ ]Skipped

taos-ci · 2024-09-13T02:17:40Z

📝 TAOS-CI Version: 1.5.20200925. Thank you for submitting PR #2735. 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/.

taos-ci · 2024-09-13T02:55:50Z

cibot: @EunjuYang, A builder checker could not be completed because one of the checkers is not completed. In order to find out a reason, please go to http://ci.nnstreamer.ai/nntrainer/ci/repo-workers/pr-checker/2735-202409131121380.57520198822021-232c022878125296a7ce2b9b809a433cd68bbff1/.

djeong20

Great work overall! Please check the comments below.

djeong20 · 2024-09-23T01:00:06Z

nntrainer/tensor/uint_tensor.cpp

+template <typename T, Tdatatype D>
+UIntTensor<T, D>::UIntTensor(std::string name_, Tformat fm) :
+  TensorBase(name_, fm, D) {}


It seems like Tdatatype D is used only in constructors. I think it could be removed.

Suggested change

template <typename T, Tdatatype D>

UIntTensor<T, D>::UIntTensor(std::string name_, Tformat fm) :

TensorBase(name_, fm, D) {}

template <typename T>

UIntTensor<T>::UIntTensor(std::string name_, Tformat fm) :

TensorBase(name_, fm, checkTensorDataType()) {}

and define a private function checkTensorDataType().

// uint_tensor.h Tdatatype checkTensorDataType() const { if (typeid(T) == typeid(uint8_t)) return Tdatatype::UINT8; else if (typeid(T) == typeid(uint16_t)) return Tdatatype::UINT16; else if (typeid(T) == typeid(uint32_t)) return Tdatatype::UINT32; else throw std::runtime_error("unsupported type"); }

Thank you for your suggestion. It sounds great! It seems much better to update the code like you recommended. I will update the code.

djeong20 · 2024-09-23T01:04:23Z

api/ccapi/include/tensor_dim.h

    UINT16, /** unsigned int 16 bit */
+    UINT32, /** unsigned int 32 bit */


please check tensor_dim.cpp to reflect this patch. (e.g., getDataTypeSize(), operator<<)

I updated the tensor_dim.cpp. Thank you for the comment.

djeong20 · 2024-09-23T01:17:42Z

test/unittest/unittest_nntrainer_tensor.cpp

+    }
+  }
+}
+


one negative test case that comes to my mind is comparing UInt16Tensor t1 and UInt32Tensor t2 with the same size and values and checking for ASSERT_NE(t1, t2).

- This commit resolves nnstreamer#2733 - This commit implements template of UIntTensor based on the ShortTensor class. - Based on the template, this commit supports UInt8 / UInt16 / UInt32 - Implement UIntTensor Template - Implement UInt8Tensor / UInt16Tensor / UInt32Tensor (ShortTensor is replaced) - Unit tests for UInt8 and UInt32 are added The `uint_tensor.cpp` file is implemented in the way to be imported in `uint_tensor.h`. Since uint_tensor.h declares class prototype with template, all its implementation should be included in one file. However, for better readability, I split the code into uint_tensor.cpp. For this reason, this code should not be included as `compile` but only used as inclusion purpose. Self evaluation: Build test: [X]Passed [ ]Failed [ ]Skipped Run test: [X]Passed [ ]Failed [ ]Skipped Signed-off-by: Eunju Yang <[email protected]>

taos-ci · 2024-09-23T06:03:17Z

cibot: @EunjuYang, nntrainer/tensor/uint_tensor.h does not include Doxygen tags such as @file @brief @author @bug. You must include the Doxygen tags in the source code. Please refer to a Doxygen manual at http://github.com/nnstreamer/TAOS-CI/blob/main/ci/doc/doxygen-documentation.md

- This commit removes some codes, including some tensor-related headers, which include tensor.h again in their implementation. - To avoid recursive inclusion, this commit removes - char_tensor.h - float_tensor.h - uint_tensor.h - In terms of this, it is better to remove half_tensor.h as well, but it raises linkage error when doing the same operation. Thus, it remains as todo. - [fix doxygen error] uint_tensor.h Self evaluation: Build test: [X]Passed [ ]Failed [ ]Skipped Run test: [X]Passed [ ]Failed [ ]Skipped Signed-off-by: Eunju Yang <[email protected]>

taos-ci

@EunjuYang, 💯 All CI checkers are successfully verified. Thanks.

taos-ci

@EunjuYang, 💯 All CI checkers are successfully verified. Thanks.

djeong20

Thank you for the update 👍 I left some comments on the new patch. PTAL!

djeong20 · 2024-09-23T23:41:33Z

nntrainer/tensor/tensor.h

@@ -203,6 +197,8 @@ class Tensor {
   * @note      This constructor copies vector again. needs refactoring
   * @param[in] d data for the Tensor with batch size one
   * @param[in] t_type Tensor Type
+   * @todo      It is more desirable to move this implementaton into
+   *            `tensor.cpp`, for it requires half_tensor.h


I see all other data type implementations have been moved to .cpp except for half tensor. could you explain why you left it as todo?

I attempted to relocate it to 'tensor.cpp' with #ifdef, but encountered an undefined symbol error during compilation (I face this error only with Android build). Thus, I decided to leave it unchanged in the current version and will investigate soon.

djeong20 · 2024-09-23T23:48:35Z

nntrainer/tensor/uint_tensor.cpp

 * @bug		No known bugs except for NYI items
 */

-#include <iomanip>
-#include <iostream>
+#ifdef __UINT_TENSOR_H__


I'm not sure whether having uint_tensor.cpp file is good or not. is there a particular reason why you preserved the cpp file?

You are correct that there is no need to separate the files, and it may actually be more natural to include everything in one header file. However, I believed doing so could enhance readability and attempted to reach a consensus with other tensor implementations. Please share your thoughts, as I plan to incorporate the reviewers' feedback into the PR.

I think there's no right answer to this. I was just curious if there are any reasons behind this and I agree with you about the benefits of having a cpp file!

djeong20

Appreciate the hard work! great job :) I think it is good to go 👍

DonghakPark

Great discussion & Changes. LGTM!!

baek2sm

great work. LGTM!

jijoongmoon · 2024-10-21T01:03:22Z

nntrainer/tensor/tensor.h

+   * @param[in] d data for the Tensor. It needs to set format properly.
+   * @param[in] t_type Tensor Type
+   */
+  Tensor(std::vector<std::vector<std::vector<std::vector<uint8_t>>>> const &d,


We might need a tensor constructer with zero and scale.

What else do we need besides the constructor with zero and scale?
Do you expect other methods or functions to improve with the parameters?

For now, Zero and Scale (their vector type may need to be supported) are the only two I can think of, but there are more. @djeong20, let's discuss it. I think this can be done in a later PR.

Another think is that we need to implement the tensor operations like add, mul, etc.

EunjuYang requested review from myungjoo, jijoongmoon, again4you, jaeyun-jung, leemgs, wooksong, helloahn, kparichay, gichan-jang, anyj0527, zhoonit, lhs8928, songgot, jihochu, DonghakPark, SeoHyungjun and baek2sm as code owners September 13, 2024 02:17

EunjuYang requested review from skykongkong8, djeong20 and a team as code owners September 13, 2024 02:17

github-actions bot added the Need Review label Sep 13, 2024

EunjuYang force-pushed the uint_tensor branch from 5220bb1 to 232c022 Compare September 13, 2024 02:21

EunjuYang mentioned this pull request Sep 23, 2024

[ Tensor ] more types of Tensors are required. #2733

Closed

djeong20 reviewed Sep 23, 2024

View reviewed changes

EunjuYang force-pushed the uint_tensor branch 2 times, most recently from 5916650 to c0d8a91 Compare September 23, 2024 06:03

EunjuYang force-pushed the uint_tensor branch from c0d8a91 to ed45986 Compare September 23, 2024 06:06

taos-ci approved these changes Sep 23, 2024

View reviewed changes

EunjuYang changed the title ~~[ Tensor ] UInt Tensor : UInt8 / UInt16 / Uint32~~ [ Tensor ] UInt Tensor : UInt8 / UInt16 / Uint32 @open sesame 09/23 16:50 Sep 23, 2024

taos-ci approved these changes Sep 23, 2024

View reviewed changes

djeong20 reviewed Sep 23, 2024

View reviewed changes

djeong20 approved these changes Sep 25, 2024

View reviewed changes

DonghakPark approved these changes Sep 25, 2024

View reviewed changes

github-actions bot added PR/READY2MERGE and removed Need Review labels Sep 25, 2024

baek2sm approved these changes Oct 14, 2024

View reviewed changes

jijoongmoon reviewed Oct 21, 2024

View reviewed changes

jijoongmoon merged commit bf7824e into nnstreamer:main Oct 23, 2024
57 of 61 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ Tensor ] UInt Tensor : UInt8 / UInt16 / Uint32 @open sesame 09/23 16:50 #2735

[ Tensor ] UInt Tensor : UInt8 / UInt16 / Uint32 @open sesame 09/23 16:50 #2735

EunjuYang commented Sep 13, 2024 •

edited

Loading

taos-ci commented Sep 13, 2024

taos-ci commented Sep 13, 2024

djeong20 left a comment

djeong20 Sep 23, 2024

EunjuYang Sep 23, 2024

djeong20 Sep 23, 2024

EunjuYang Sep 23, 2024

djeong20 Sep 23, 2024

taos-ci commented Sep 23, 2024

taos-ci left a comment

taos-ci left a comment

djeong20 left a comment

djeong20 Sep 23, 2024

EunjuYang Sep 24, 2024 •

edited

Loading

djeong20 Sep 23, 2024

EunjuYang Sep 24, 2024

djeong20 Sep 25, 2024

djeong20 left a comment

DonghakPark left a comment

baek2sm left a comment

jijoongmoon Oct 21, 2024

EunjuYang Oct 21, 2024

jijoongmoon Oct 23, 2024

jijoongmoon Oct 23, 2024

		UINT16, /** unsigned int 16 bit */
		UINT32, /** unsigned int 32 bit */

[ Tensor ] UInt Tensor : UInt8 / UInt16 / Uint32 @open sesame 09/23 16:50 #2735

[ Tensor ] UInt Tensor : UInt8 / UInt16 / Uint32 @open sesame 09/23 16:50 #2735

Conversation

EunjuYang commented Sep 13, 2024 • edited Loading

taos-ci commented Sep 13, 2024

taos-ci commented Sep 13, 2024

djeong20 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taos-ci commented Sep 23, 2024

taos-ci left a comment

Choose a reason for hiding this comment

taos-ci left a comment

Choose a reason for hiding this comment

djeong20 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EunjuYang Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djeong20 left a comment

Choose a reason for hiding this comment

DonghakPark left a comment

Choose a reason for hiding this comment

baek2sm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EunjuYang commented Sep 13, 2024 •

edited

Loading

EunjuYang Sep 24, 2024 •

edited

Loading