Skip to content
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

[Layer] Remove Tensor setDataType() usage #2498

Merged
merged 1 commit into from
Mar 15, 2024

Conversation

djeong20
Copy link
Contributor

@djeong20 djeong20 commented Mar 7, 2024

In several layers, there are attempts to change the data type of a Tensor object after initializing it.
This is currently possible but can cause issues down the line (e.g., treat FloatTensor object as HalfTensor).
As such, the setDataType() method will be removed and considered not to be used in future updates.
Instead, users will need to provide the desired data type when creating a new tensor.

Self-evaluation:

  1. Build test: [X]Passed [ ]Failed [ ]Skipped
  2. Run test: [X]Passed [ ]Failed [ ]Skipped

@taos-ci
Copy link

taos-ci commented Mar 7, 2024

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

Copy link

@taos-ci taos-ci left a 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.

In several layers, there are attempts to change the data type of a Tensor object after initializing it.
This is currently possible but can cause issues down the line (e.g., treat FloatTensor object as HalfTensor).
As such, the setDataType() method will be removed and considered not to be used in future updates.
Instead, users will need to provide the desired data type when creating a new tensor.

**Self-evaluation:**
1. Build test: [X]Passed [ ]Failed [ ]Skipped
2. Run test:   [X]Passed [ ]Failed [ ]Skipped

Signed-off-by: Donghyeon Jeong <[email protected]>
@djeong20 djeong20 force-pushed the fix/layer/setDataType branch from a477abe to 044ba5b Compare March 8, 2024 05:04
Copy link

@taos-ci taos-ci left a 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.

Copy link
Member

@skykongkong8 skykongkong8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think this is quite important point.
I will keep this in mind in the future development of myself as well :)

@djeong20 djeong20 changed the title [Layer] Remove Tensor setDataType() usuage [Layer] Remove Tensor setDataType() usage Mar 12, 2024
@myungjoo
Copy link
Member

Question:

In a network, is the Tensor name required to be unique?

  Tensor tensor =
    Tensor("some_name", value.getFormat(), value.getDataType());

(I hope not..)

Tensor empty;
empty.setTensorType(weight_tensor_type);
Tensor empty =
Tensor("empty", weight_tensor_type.format, weight_tensor_type.data_type);
Copy link
Collaborator

@jijoongmoon jijoongmoon Mar 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can set the format and data_type with weight_tensor_type. No need to set separately. Also, we do not need to set the tensor name always. I think when we create tensor like here, Tensor empty, then it does not allocate the buffer. So.. I think it is ok to set the Tensor type here. The reason we set like this.. we do not want to allocate a buffer at this time.

Copy link
Contributor Author

@djeong20 djeong20 Mar 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we discussed offline, Tensor empty; would create FloatTensor as a default.

nntrainer::Tensor t;
if (t.getDataType() == nntrainer::Tdatatype::FP32) {
  // This would be executed
  std::cout << "FP32" << std::endl;
} else {
  std::cout << "FP16" << std::endl;
}

I will make an additional Tensor constructor that only takes data type without allocating a buffer (e.g., Tensor empty = Tensor(weight_tensor_type.data_type);)

@djeong20
Copy link
Contributor Author

Question:

In a network, is the Tensor name required to be unique?

  Tensor tensor =
    Tensor("some_name", value.getFormat(), value.getDataType());

(I hope not..)

I don't think the name has to be unique.

@myungjoo
Copy link
Member

Question:
In a network, is the Tensor name required to be unique?

  Tensor tensor =
    Tensor("some_name", value.getFormat(), value.getDataType());

(I hope not..)

I don't think the name has to be unique.

If this is for debugging and developer referencing, it's ok.

Copy link
Collaborator

@jijoongmoon jijoongmoon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jijoongmoon jijoongmoon merged commit 6be8e84 into nnstreamer:main Mar 15, 2024
36 checks passed
@djeong20 djeong20 deleted the fix/layer/setDataType branch March 20, 2024 01:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants