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] add "add layer" #2710

Merged
merged 1 commit into from
Nov 9, 2024
Merged

Conversation

baek2sm
Copy link
Contributor

@baek2sm baek2sm commented Aug 16, 2024

  • added "add layer"
  • added a "model unit test" for add layer. Since 'model-level unit test'
    havn't been run for a long time, I diabled some test cases that causing
    issues when running model unit test.

Many people gave great feedback, so I've improved the structure accordingly.

  • An upper class called "OperationLayer" was added to reduce redundant code.

  • Based on the number of input tensors, the behavior of "OperationLayer" has been classified into two types: unary and binary operations.

  • Various additional code cleanups have also taken place.

  • there is an issue where committing compressed files containing golden data for unit test prevents pushing changes to the remote server. (I've confirmed that all unit tests pass locally using that golden data.)

Self evaluation:

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

Signed-off-by: Seungbaek Hong [email protected]

@taos-ci
Copy link
Collaborator

taos-ci commented Aug 16, 2024

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

@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.

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

Copy link
Member

@SeoHyungjun SeoHyungjun left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@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.

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

Copy link
Contributor

@lhs8928 lhs8928 left a comment

Choose a reason for hiding this comment

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

I can't find the difference with addition layer.
Could you explain why did you make a add layer even though NNTrainer already has a addition layer?

Tensor &hidden_ = context.getOutput(SINGLE_INOUT_IDX);

const Tensor &input0 = context.getInput(0);
const Tensor &input1 = context.getInput(1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Does add layer only has 2 inputs?
Is there any possibility of having more than 2 inputs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lhs8928 Good point! The addition layer, to be precise, implements a summation operation. This layer corresponds to four basic operations that take two inputs.

Comment on lines 60 to 66
const Tensor &input0 = context.getInput(0);
const Tensor &input1 = context.getInput(1);

TensorDim input_dim = input0.getDim();
TensorDim input_step_dim = input_dim;
input_step_dim.batch(1);
input_step_dim.height(to - from);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to add this code inside the batch loop?
What about moving the codes to the outside of the loop ?

@taos-ci
Copy link
Collaborator

taos-ci commented Oct 7, 2024

:octocat: cibot: @baek2sm, nntrainer/layers/operation_layer.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

@taos-ci
Copy link
Collaborator

taos-ci commented Oct 7, 2024

:octocat: cibot: @baek2sm, 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/2710-202410072102570.53899788856506-4eb6d12630d2c098e809c2afeeb989f24a7e72aa/.

@taos-ci
Copy link
Collaborator

taos-ci commented Oct 8, 2024

:octocat: cibot: @baek2sm, 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/2710-202410081320430.68198895454407-cc20fb9b4796d1cc9670a75a05e3790a15921ba6/.

@taos-ci
Copy link
Collaborator

taos-ci commented Oct 8, 2024

:octocat: cibot: @baek2sm, 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/2710-202410081705590.20722198486328-6d958fffa24022aa7a16283587c4f3ecfacf3392/.

Copy link
Collaborator

@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.

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

@baek2sm baek2sm force-pushed the n_op_add_layer branch 3 times, most recently from d9416db to 8600446 Compare October 10, 2024 06:12
Copy link
Collaborator

@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.

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

* @brief Type of layer depending on the number of inputs.
*
*/
enum class OperationType { NONE, UNARY, BINARY };
Copy link
Collaborator

Choose a reason for hiding this comment

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

It is important to consider whether the operation can be performed in-place to save memory. I guess it might be good to give the user an option to set it when the operation layer is created.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A new pull request will be uploaded concerning inplace operations. This implementation has been updated based on the UnaryOperationLayer and BinaryOperationLayer classes! Thanks

@taos-ci
Copy link
Collaborator

taos-ci commented Oct 29, 2024

:octocat: cibot: @baek2sm, 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/2710-202410291050240.86144089698792-8c108e65df8b71f4f0aae6405045fb3fe672a6a1/.

- added "add layer"
- added a "model unit test" for add layer. Since 'model-level unit test'
havn't been run for a long time, I diabled some test cases that causing
issues when running model unit test.

Many people gave great feedback, so I've improved the structure accordingly.
- An upper class called "OperationLayer" was added to reduce redundant code.
- Based on the number of input tensors, the behavior of "OperationLayer" has been classified into two types: unary and binary operations.
- Various additional code cleanups have also taken place.

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

Signed-off-by: Seungbaek Hong <[email protected]>
Copy link
Collaborator

@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.

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

@baek2sm baek2sm changed the title [Layer] add "add layer" @open sesame 08/30 11:02 [WIP][Layer] add "add layer" @open sesame 08/30 11:02 Oct 30, 2024
@baek2sm baek2sm added the WIP label Oct 30, 2024
Copy link
Collaborator

@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.

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

@baek2sm baek2sm changed the title [WIP][Layer] add "add layer" @open sesame 08/30 11:02 [Layer] add "add layer" @open sesame 08/30 11:02 Nov 6, 2024
@baek2sm baek2sm removed the WIP label Nov 6, 2024
@baek2sm baek2sm changed the title [Layer] add "add layer" @open sesame 08/30 11:02 [Layer] add "add layer" Nov 6, 2024
Copy link
Collaborator

@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.

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

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 b7e5579 into nnstreamer:main Nov 9, 2024
56 checks passed
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.

8 participants