-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Avoid duplicate evaluations in chaining comparison #540
Conversation
…umber of args (taichi-dev#534) * Make sure KernelTemplateMapper extractors's size is the same as the number of args Fixes taichi-dev#531 * fix test
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.
Thank you Mingkuan! The implementation is very well-done. Great job!
The only minor place to improve is to break the tests into multiple pieces. Please see the standalone comment. This PR will be merged once the tests are improved.
|
||
@ti.kernel | ||
def func(): | ||
a[2] = 0 <= ti.append(a.parent(), [], 10) < 1 |
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.
I would break the test into multiple pieces.
a[2] = 0 <= ti.append(a.parent(), [], 10) < 1
itself can be considered as one test (test_no_duplicate_eval
).a[3] = b < c == d
toc == d != b < d > b >= b <= c
(test_chain_compare
)- It is also worth testing all 6 ops independently, so that every line of your code written is covered. (
test_compare_basics
) - Also test the difference between, say,
<
and<=
. (test_compare_equality
)
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.
Awesome! Will merge once CI passes.
CI fails at some tests which seem not my fault? |
Please do not apply the AST transformer (all transforms including |
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.
Looks good!! Let's see if all tests pass this time. Thanks!
|
Yeah now it may be throwing a different kind of exception. Please update |
Now it successfully finished without throwing any exceptions. |
I see. Please disallow |
How can I know if the scope is Taichi-scope in expr.py? |
|
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.
Great job! Merging now!
Related issue id = #327