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.

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

Merged
merged 15 commits into from
Feb 26, 2020

Conversation

xumingkuan
Copy link
Collaborator

Related issue id = #327

Copy link
Member

@yuanming-hu yuanming-hu left a 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
Copy link
Member

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 to c == 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)

Copy link
Member

@yuanming-hu yuanming-hu left a 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.

@xumingkuan
Copy link
Collaborator Author

CI fails at some tests which seem not my fault?

@yuanming-hu
Copy link
Member

yuanming-hu commented Feb 26, 2020

CI fails at some tests which seem not my fault?

Please do not apply the AST transformer (all transforms including visit_Compare) to nodes inside if ti.static(...) test expressions. A potentially working solution is to change visit_Call: if the function called is ti.static, then do no apply the generic visitor to its argument.

Copy link
Member

@yuanming-hu yuanming-hu left a 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!

@xumingkuan
Copy link
Collaborator Author

test_static_if_error fails :(

@yuanming-hu
Copy link
Member

Yeah now it may be throwing a different kind of exception. Please update @ti.must_throw(AssertionError)

@xumingkuan
Copy link
Collaborator Author

Now it successfully finished without throwing any exceptions.

@yuanming-hu
Copy link
Member

yuanming-hu commented Feb 26, 2020

I see. Please disallow Expr.__getitem__ and Matrix.__getitem__ in Taichi-scope then.

@xumingkuan
Copy link
Collaborator Author

How can I know if the scope is Taichi-scope in expr.py?

@yuanming-hu
Copy link
Member

ti.get_runtime().inside_kernel

Copy link
Member

@yuanming-hu yuanming-hu left a 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!

@yuanming-hu yuanming-hu merged commit 19c5d91 into taichi-dev:master Feb 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants