-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
[Fix][TIR] Fix tvm::arith::UnionLowerBound #14304
Conversation
Thanks for contributing to TVM! Please refer to the contributing guidelines https://tvm.apache.org/docs/contribute/ for useful information and tips. Please request code reviews from Reviewers by @-ing them in a comment.
Generated by tvm-bot |
Could you please add a regression test? |
We could add a testcase to tests/python/unittest/test_arith_intset.py::test_union_lower_bound |
Already added, thanks for your suggestion. |
Already added, thanks for your suggestion |
@@ -370,7 +370,8 @@ def test_union_lower_bound(): | |||
pos_inf = tvm.arith.int_set.pos_inf() | |||
set_0 = tvm.arith.IntervalSet(min_value=neg_inf, max_value=0) | |||
set_1 = tvm.arith.IntervalSet(min_value=1, max_value=pos_inf) | |||
result = tvm.arith.int_set.union_lower_bound([set_0, set_1]) | |||
set_2 = tvm.arith.IntervalSet(min_value=pos_inf, max_value=neg_inf) | |||
result = tvm.arith.int_set.union_lower_bound([set_0, set_1, set_2]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is better to preserve original case and just append new cases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code updated,thanks for your great advice!
@@ -373,6 +373,8 @@ def test_union_lower_bound(): | |||
result = tvm.arith.int_set.union_lower_bound([set_0, set_1]) | |||
assert result.min_value.same_as(neg_inf) | |||
assert result.max_value.same_as(pos_inf) | |||
set_2 = tvm.arith.IntervalSet(min_value=pos_inf, max_value=neg_inf) | |||
result = tvm.arith.int_set.union_lower_bound([set_0, set_2]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing final result assertions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code updated,thanks for your great advice!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look good to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make the CI green and we can get it in :-)
@tvm-bot re-run |
Hi, CI is finished. |
The UnionLowerBound function does not take into account the condition that the empty set has a special representation [+inf, -inf].
cc @wrongtest-intellif