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

[luci/service] migrate Range shape inference rule to sinf::Algorithm #13987

Merged
merged 6 commits into from
Sep 12, 2024
Merged

[luci/service] migrate Range shape inference rule to sinf::Algorithm #13987

merged 6 commits into from
Sep 12, 2024

Conversation

kyeong8139
Copy link
Contributor

This commit migrate Range shape inference rule to sinf::Algorithm.
Todo : support shape inference for non-const param of Range

ONE-DCO-1.0-Signed-off-by: Bokyeong Lee [email protected]

@kyeong8139
Copy link
Contributor Author

Draft : [luci/service] support shape inference for non-const param of Range
For: Shape inference for operators whose inputs have dynamic shape
Related: [one-optimize] YAMNet Optimization Not Working

@kyeong8139
[luci/service] migrate Range shape inference rule to sinf::Algorithm
ffa0ce8 
This commit migrate Range shape inference rule to sinf::Algorithm.

ONE-DCO-1.0-Signed-off-by: bokyeong Lee <[email protected]>
@kyeong8139 kyeong8139 added SSAFY PR/ready for review It is ready to review. Please review it. labels Sep 11, 2024
@kyeong8139
[luci/service] add include
a1d4c75 
This commit add include code.

ONE-DCO-1.0-Signed-off-by: bokyeong lee <[email protected]>
@kyeong8139 kyeong8139 requested review from a team September 11, 2024 06:48
@kyeong8139
[luci/service] copy own_shape function with non-const params
1f3e0fd 
This commit copy own_shape function with non-contst params.

ONE-DCO-1.0-Signed-off-by: bokyeong Lee <[email protected]>
zetwhite
zetwhite reviewed Sep 11, 2024
View reviewed changes
Copy link
Contributor

@zetwhite zetwhite left a comment

Choose a reason for hiding this comment

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

Could you copy and paste the testcase (TEST(ShapeRuleTest, range_const_param)) from your draft?

IMO, It might be good to add a test to show that original algorithm works well for CircleConst inputs.

@kyeong8139
[luci/service] add tests for range with const param
297a187 
This commit add tests for range with const param.

ONE-DCO-1.0-Signed-off-by: bokyeong Lee <[email protected]>
@kyeong8139
Copy link
Contributor Author

I add test cases to show that original algorithm works well for CircleConst inputs in zenwhite's opinion.
and add comment for prevent confusion

zetwhite
zetwhite previously approved these changes Sep 11, 2024
View reviewed changes
Copy link
Contributor

@zetwhite zetwhite left a comment

Choose a reason for hiding this comment

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

LGTM 😄

@kyeong8139
[luci/service] remove comment
5c474bc 
This commit remove comment.

ONE-DCO-1.0-Signed-off-by: bokyeong Lee <[email protected]>
@kyeong8139 @seanshpark
[luci/service] revise comment
2ea01be 
This commit revise comment about use_own()

ONE-DCO-1.0-Signed-off-by: Bokyeong Lee k<[email protected]>

Co-authored-by: SaeHie Park <[email protected]>
@seanshpark seanshpark mentioned this pull request Sep 11, 2024
Merged
shs-park
shs-park approved these changes Sep 11, 2024
View reviewed changes
Copy link
Contributor

@shs-park shs-park left a comment

Choose a reason for hiding this comment

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

LGTM!
=)

@zetwhite zetwhite requested a review from seanshpark September 12, 2024 02:30
llFreetimell
llFreetimell approved these changes Sep 12, 2024
View reviewed changes
Copy link
Contributor

@llFreetimell llFreetimell left a comment

Choose a reason for hiding this comment

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

LGTM

seanshpark
seanshpark approved these changes Sep 12, 2024
View reviewed changes
Copy link
Contributor

@seanshpark seanshpark left a comment

Choose a reason for hiding this comment

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

LGTM thank you!

@seanshpark seanshpark merged commit 3a3e50b into Samsung:master Sep 12, 2024
10 checks passed
@kyeong8139 kyeong8139 deleted the luci/range branch September 12, 2024 04:04
@kyeong8139 kyeong8139 mentioned this pull request Sep 12, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shs-park shs-park shs-park approved these changes

@zetwhite zetwhite zetwhite approved these changes

@llFreetimell llFreetimell llFreetimell approved these changes

@seanshpark seanshpark seanshpark approved these changes

Assignees
No one assigned
Labels
PR/ready for review It is ready to review. Please review it. SSAFY
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kyeong8139 @seanshpark @llFreetimell @zetwhite @shs-park