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

Implements ToPythonASTMapper #107

Merged
merged 5 commits into from
Aug 2, 2022
Merged

Conversation

kaushikcfd
Copy link
Collaborator

No description provided.

@kaushikcfd kaushikcfd mentioned this pull request Jul 28, 2022
1 task
@inducer
Copy link
Owner

inducer commented Jul 28, 2022

What's your take on the CI failures?

@kaushikcfd
Copy link
Collaborator Author

What's your take on the CI failures?

Was just fixing those! :)

@kaushikcfd kaushikcfd force-pushed the to_ast_mapper branch 4 times, most recently from 9380b52 to ad9ff3c Compare July 28, 2022 17:54
@kaushikcfd
Copy link
Collaborator Author

@inducer: This is ready.
(TIL the ast package is awfully unstable across versions.)

@inducer
Copy link
Owner

inducer commented Jul 28, 2022

Thanks! Could you say a bit about the motivation for this, and why it's worth it to deal with the instability of ast?

@kaushikcfd
Copy link
Collaborator Author

kaushikcfd commented Jul 28, 2022

Thanks! Could you say a bit about the motivation for this, and why it's worth it to deal with the instability of last?

In PyOP2, we need to cache a part of the Loopy's "host code" on the disk (to be able to compute the computational grid sizes). The other option was to use genpy for it. But I've generally felt that ast is more complete and comes with extra batteries to process these trees.

@inducer
Copy link
Owner

inducer commented Aug 2, 2022

The discussion in OP2/PyOP2#574 (comment) has echos of inducer/loopy#650 to me, in the sense that it's indicative of a potentially missing "invoker" level, where these (presumably) grid sizes likely should reside. Could you explain a bit more how they're used?

(Commented there instead of here, since that discussion has very little to do with this PR.)

@inducer
Copy link
Owner

inducer commented Aug 2, 2022

Thx!

@inducer inducer merged commit 646c9de into inducer:main Aug 2, 2022
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.

2 participants