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.

Sign up for GitHub

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

Jump to bottom

[Wait for #2710][Layer] add "subtract layer" @open sesame 11/06 18:29 #2724

Merged
merged 1 commit into from
Nov 11, 2024
Merged

[Wait for #2710][Layer] add "subtract layer" @open sesame 11/06 18:29 #2724

merged 1 commit into from
Nov 11, 2024

Conversation

baek2sm
Copy link
Contributor

@baek2sm baek2sm commented Aug 30, 2024 •
edited
Loading

  • added "subtract layer"

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

taos-ci commented Aug 30, 2024

📝 TAOS-CI Version: 1.5.20200925. Thank you for submitting PR #2724. 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
taos-ci approved these changes Aug 30, 2024
View reviewed changes
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.

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

SeoHyungjun
SeoHyungjun approved these changes Sep 2, 2024
View reviewed changes
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.

The add and sub layers have almost identical code.
How about creating a basic operation class?
It would be simple if the add and sub inherited from the basic operation class.
Multiplication and division also seem to have the same structure.

skykongkong8
skykongkong8 approved these changes Sep 2, 2024
View reviewed changes
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.

LGTM

djeong20
djeong20 approved these changes Sep 2, 2024
View reviewed changes
Copy link
Contributor

@djeong20 djeong20 left a comment

Choose a reason for hiding this comment

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

Nice work! One minor suggestion is to rename it SubtractLayer (SubLayer sounds like a subunit of a layer). What do you think?

@baek2sm
Copy link
Contributor Author

baek2sm commented Sep 2, 2024

The add and sub layers have almost identical code. How about creating a basic operation class? It would be simple if the add and sub inherited from the basic operation class. Multiplication and division also seem to have the same structure.

That sounds good. Let's think together to find an improved structure.

taos-ci
taos-ci approved these changes Oct 10, 2024
View reviewed changes
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.

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

@baek2sm
Copy link
Contributor Author

baek2sm commented Oct 10, 2024

I've completed the revisions after incorporating feedbacks. Thanks a lot!

taos-ci
taos-ci approved these changes Oct 10, 2024
View reviewed changes
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.

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

taos-ci
taos-ci approved these changes Oct 11, 2024
View reviewed changes
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.

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

@baek2sm baek2sm changed the title [Wait for #2710][Layer] add "sub layer" @open sesame 10/10 16:43 [Wait for #2710][Layer] add "subtract layer" @open sesame 10/10 16:43 Oct 11, 2024
@baek2sm baek2sm changed the title [Wait for #2710][Layer] add "subtract layer" @open sesame 10/10 16:43 [Wait for #2710][Layer] add "subtract layer" @open sesame 10/11 15:45 Oct 11, 2024
taos-ci
taos-ci approved these changes Oct 11, 2024
View reviewed changes
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.

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

taos-ci
taos-ci approved these changes Oct 11, 2024
View reviewed changes
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.

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

@baek2sm baek2sm added the WIP label Nov 6, 2024
@baek2sm baek2sm changed the title [Wait for #2710][Layer] add "subtract layer" @open sesame 10/11 15:45 [WIP][Wait for #2710][Layer] add "subtract layer" @open sesame 10/11 15:45 Nov 6, 2024
@baek2sm baek2sm changed the title [WIP][Wait for #2710][Layer] add "subtract layer" @open sesame 10/11 15:45 [Wait for #2710][Layer] add "subtract layer" Nov 6, 2024
@baek2sm baek2sm removed the WIP label Nov 6, 2024
@taos-ci
Copy link

taos-ci commented Nov 6, 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/2724-202411061131060.035284996032715-13091f071b159bb56d9d5e3b71eefb7a4805066e/.

@baek2sm baek2sm changed the title [Wait for #2710][Layer] add "subtract layer" [Wait for #2710][Layer] add "subtract layer" @open sesame 11/06 16:22 Nov 6, 2024
@baek2sm baek2sm force-pushed the n_op_sub_layer branch 2 times, most recently from 592c7e9 to f3aad56 Compare November 6, 2024 07:45
@taos-ci
Copy link

taos-ci commented Nov 6, 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/2724-202411061645350.87372088432312-f3aad560a9d01bba11c3cfc53c0dee8b2ad54f7d/.

@baek2sm baek2sm changed the title [Wait for #2710][Layer] add "subtract layer" @open sesame 11/06 16:22 [Wait for #2710][Layer] add "subtract layer" @open sesame 11/06 18:29 Nov 6, 2024
taos-ci
taos-ci approved these changes Nov 6, 2024
View reviewed changes
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.

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

@baek2sm
[Layer] add "subtract layer"
8125cbe 
- added "subtract layer"
- added a "model unit test" for sub layer.

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.

**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
taos-ci approved these changes Nov 11, 2024
View reviewed changes
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.

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

djeong20
djeong20 approved these changes Nov 11, 2024
View reviewed changes
Copy link
Contributor

@djeong20 djeong20 left a comment

Choose a reason for hiding this comment

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

nice work!

@jijoongmoon jijoongmoon merged commit 6a06ec6 into nnstreamer:main Nov 11, 2024
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@EunjuYang EunjuYang EunjuYang left review comments

@skykongkong8 skykongkong8 skykongkong8 approved these changes

@djeong20 djeong20 djeong20 approved these changes

@taos-ci taos-ci taos-ci approved these changes

@SeoHyungjun SeoHyungjun SeoHyungjun approved these changes

@myungjoo myungjoo Awaiting requested review from myungjoo myungjoo is a code owner

@jijoongmoon jijoongmoon Awaiting requested review from jijoongmoon jijoongmoon is a code owner

@again4you again4you Awaiting requested review from again4you again4you is a code owner

@jaeyun-jung jaeyun-jung Awaiting requested review from jaeyun-jung jaeyun-jung is a code owner

@leemgs leemgs Awaiting requested review from leemgs leemgs is a code owner

@wooksong wooksong Awaiting requested review from wooksong wooksong is a code owner

@helloahn helloahn Awaiting requested review from helloahn

@kparichay kparichay Awaiting requested review from kparichay

@gichan-jang gichan-jang Awaiting requested review from gichan-jang gichan-jang is a code owner

@anyj0527 anyj0527 Awaiting requested review from anyj0527 anyj0527 is a code owner

@zhoonit zhoonit Awaiting requested review from zhoonit

@lhs8928 lhs8928 Awaiting requested review from lhs8928 lhs8928 is a code owner

@songgot songgot Awaiting requested review from songgot songgot is a code owner

@jihochu jihochu Awaiting requested review from jihochu jihochu is a code owner

@DonghakPark DonghakPark Awaiting requested review from DonghakPark DonghakPark is a code owner

Assignees
No one assigned
Labels
rebase required
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@baek2sm @taos-ci @jijoongmoon @SeoHyungjun @EunjuYang @djeong20 @skykongkong8