Feat: Finalize Polyhedral Compiler Stuff#17
Conversation
|
🤖 Hi @hikettei, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
Summary of ChangesHello @hikettei, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on finalizing the polyhedral compiler examples by updating and refactoring the associated Jupyter notebooks. The changes ensure that the examples are executable, their outputs are captured for immediate viewing, and the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
🤖 I'm sorry @hikettei, but I was unable to process your request. Please see the logs for more details. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates two example notebooks by executing the code cells and committing the outputs. While this is helpful for isl_tutorial.ipynb, the polyhedral_compiler.ipynb has been significantly refactored, removing the complete examples for matrix multiplication and convolution fusion. It now only contains helper function definitions without any usage, making the example incomplete. It's unclear if this is intentional, but it reduces the value of the notebook as a demonstration. Additionally, both notebooks now modify sys.path to resolve imports, which is not ideal. I've added specific comments with suggestions for improvement.
| "import sys\n", | ||
| "sys.path.append(\"../\")\n", |
There was a problem hiding this comment.
Modifying sys.path at runtime is generally discouraged as it can lead to unexpected behavior and makes the code less portable. A better approach for running example notebooks is to either:
- Run the Jupyter server from the root of the repository. This way, the
catenpackage will be in the Python path. - Install the package in editable mode using
pip install -e .from the repository root.
This avoids runtime modifications to the path.
| "import sys\n", | ||
| "sys.path.append(\"../\")\n", |
There was a problem hiding this comment.
Modifying sys.path at runtime is generally discouraged as it can lead to unexpected behavior and makes the code less portable. A better approach for running example notebooks is to either:
- Run the Jupyter server from the root of the repository. This way, the
catenpackage will be in the Python path. - Install the package in editable mode using
pip install -e .from the repository root.
This avoids runtime modifications to the path.
examples/polyhedral_compiler.ipynb
Outdated
| "# Pool Domain: Output of Pool is H/2 x W/2\n", | ||
| "H_out, W_out = H // Pool_S, W // Pool_S\n", | ||
| "pool_dom = f\"{{ Pool[n, c, h, w] : 0 <= n < {N} and 0 <= c < {C} and 0 <= h < {H_out} and 0 <= w < {W_out} }}\" # noqa\n", | ||
| "def create_conv_schedule(N, K_out, H_out, W_out, Cin, KH, KW):\n", |
There was a problem hiding this comment.
The parameter name Cin is inconsistent with other parameters like K_out, H_out, etc. To improve consistency and readability, consider renaming it to C_in. This would align with the naming convention used for other dimensions in this context.
def create_conv_schedule(N, K_out, H_out, W_out, C_in, KH, KW):
examples/polyhedral_compiler.ipynb
Outdated
| "H_out, W_out = H // Pool_S, W // Pool_S\n", | ||
| "pool_dom = f\"{{ Pool[n, c, h, w] : 0 <= n < {N} and 0 <= c < {C} and 0 <= h < {H_out} and 0 <= w < {W_out} }}\" # noqa\n", | ||
| "def create_conv_schedule(N, K_out, H_out, W_out, Cin, KH, KW):\n", | ||
| " dom_str = f\"{{ S_conv[n, k, h, w, c, kh, kw] : 0<=n<{N} and 0<=k<{K_out} and 0<=h<{H_out} and 0<=w<{W_out} and 0<=c<{Cin} and 0<=kh<{KH} and 0<=kw<{KW} }}\"\n", |
There was a problem hiding this comment.
This line is very long and hard to read. For better readability, consider breaking it into multiple lines using parentheses.
dom_str = (
f"{{ S_conv[n, k, h, w, c, kh, kw] : "
f"0<=n<{N} and 0<=k<{K_out} and 0<=h<{H_out} and 0<=w<{W_out} and "
f"0<=c<{Cin} and 0<=kh<{KH} and 0<=kw<{KW} }}"
)
examples/polyhedral_compiler.ipynb
Outdated
| "We can use the `compute_at` method to automatically embed the Producer (Conv) schedule into the Consumer (Pool) schedule, based on dependency analysis.\n" | ||
| "def create_pool_schedule(N, K_out, H_pool, W_pool, S_pool, KH_pool, KW_pool):\n", | ||
| " # Pool Domain\n", | ||
| " dom_str = f\"{{ S_pool[n, k, h, w, rh, rw] : 0<=n<{N} and 0<=k<{K_out} and 0<=h<{H_pool} and 0<=w<{W_pool} and 0<=rh<{KH_pool} and 0<=rw<{KW_pool} }}\"\n", |
There was a problem hiding this comment.
Similar to the create_conv_schedule function, this line is quite long. Breaking it into multiple lines would improve readability.
dom_str = (
f"{{ S_pool[n, k, h, w, rh, rw] : "
f"0<=n<{N} and 0<=k<{K_out} and 0<=h<{H_pool} and 0<=w<{W_pool} and "
f"0<=rh<{KH_pool} and 0<=rw<{KW_pool} }}"
)
1 participant
Uh oh!
There was an error while loading. Please reload this page.