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

[Lang] Support template arguments for ti.func #1043

Merged
merged 9 commits into from May 24, 2020

Conversation

KLozes
Copy link
Collaborator

@KLozes KLozes commented May 23, 2020

This PR implements support for template arguments in ti.funcs. This just required skipping python AST transform for arguments type-hinted with ti.template(). I also slightly refactored ASTTransformer by removing the is_classfunc argument.

The test I added doesn't work atm. For some reason it runs into a python sytnax error. This syntax error does not occur when the test is run as stand alone script. So I think its related some weird nested function syntax. If you have another idea for the test to get around that please let me know!

Related issue = #1042

[Click here for the format server]

@archibate
Copy link
Collaborator

Cool! With function argument templates, we can easily use SNode as arguments (in #824), right? If so, could you also add this usage to test as well?
Setting up type-hint system for functions is also a big step in #602, many thanks!

@KLozes
Copy link
Collaborator Author

KLozes commented May 23, 2020

No problem! But cant we already pass snodes into funcs without ti.template(), like we did in #824 with ia = draw_rect(qt.parent(r+1), i, j, s, k, 1, 0)?

@archibate
Copy link
Collaborator

Yes, we already can, but requires a dirty hack (is it?), see 0139195#r39384288.

@KLozes
Copy link
Collaborator Author

KLozes commented May 23, 2020

Actually, I just tried commenting out elif isinstance(rhs, ti.SNode): return rhs and quadtree.py ran just fine. What was that hack needed for exactly?

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! LGTM. I suggest adding a couple of lines of comments in case we forget what's happening here in the future.

The test I added doesn't work atm. For some reason it runs into a python sytnax error. This syntax error does not occur when the test is run as stand alone script. So I think its related some weird nested function syntax. If you have another idea for the test to get around that please let me know!

Is this still an issue? It seems to me that CI tests are passing.

Comment on lines +644 to 645
if isinstance(self.func.arguments[i], ti.template):
continue
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if isinstance(self.func.arguments[i], ti.template):
continue
# Directly pass in template arguments,
# such as class instances ("self"), tensors, SNodes, etc.
if isinstance(self.func.arguments[i], ti.template):
continue
# Create a copy for non-template arguments,
# so that they are passed by value.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Aliright thanks! Yes, both tests are passing. Although the second one passes even without making x a template. I assume its related to the hack 0139195#r39384288 @archibate was talking about, which I think we can get rid of at some point.

@KLozes KLozes merged commit df26aa3 into taichi-dev:master May 24, 2020
@KLozes KLozes deleted the func_template branch May 24, 2020 21:06
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

4 participants