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

[FSU][Bugfix] Bugfix on FSU Inference #2852

Merged
merged 1 commit into from
Jan 2, 2025
Merged

[FSU][Bugfix] Bugfix on FSU Inference #2852

merged 1 commit into from
Jan 2, 2025

Conversation

DonghakPark
Copy link
Member

at case of inference there are some bug

  • when set lifespan to tensor swap & non-swap value is exchanged
  • set swap case - forward_func_lifespan
  • set non-swap case - forward_infer_lifespan

Self evaluation:

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

@DonghakPark @jijoongmoon
[FSU] Bugfix on FSU Inference
2e37844 
at case of inference there are some bug
- when set lifespan to tensor swap & non-swap value is exchanged
- set swap case - forward_func_lifespan
- set non-swap case - forward_infer_lifespan

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

Signed-off-by: Donghak PARK <[email protected]>
Co-authored-by: Jijoong Moon <[email protected]>
@DonghakPark DonghakPark added the bug Something isn't working label Jan 2, 2025
myungjoo
myungjoo reviewed Jan 2, 2025
View reviewed changes
Copy link
Member

@myungjoo myungjoo left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Need a unit test case that verifies this fix.

Reason 1: prevent regression
Reason 2: verify that this patch is really required for bugfix. (or a case that verifies that this is a bug)

myungjoo
myungjoo approved these changes Jan 2, 2025
View reviewed changes
Copy link
Member

@myungjoo myungjoo left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM. But please consider a unit test case for this. (after this PR)

@DonghakPark
Copy link
Member Author

Need a unit test case that verifies this fix.

Reason 1: prevent regression Reason 2: verify that this patch is really required for bugfix. (or a case that verifies that this is a bug)

good point !!

As suggested, I'll make unit test cases for accurate operation verification.
more detail explain : in the case of INFER_LIFESPAN, all weights are set at once for speed purposes. However, since only the necessary amount needs to be configured during swap option enable, I reported this as a bug.

EunjuYang
EunjuYang approved these changes Jan 2, 2025
View reviewed changes
Copy link
Contributor

@EunjuYang EunjuYang 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 approved these changes Jan 2, 2025
View reviewed changes
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 caef1fc into nnstreamer:main Jan 2, 2025
23 checks passed
@DonghakPark DonghakPark deleted the FSU_bugfix branch January 13, 2025 04:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jijoongmoon jijoongmoon jijoongmoon approved these changes

@EunjuYang EunjuYang EunjuYang approved these changes

@myungjoo myungjoo myungjoo approved these changes

@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

@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

@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

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

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

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

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

Assignees
No one assigned
Labels
bug Something isn't working PR/READY2MERGE
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@DonghakPark @myungjoo @EunjuYang @jijoongmoon