-
Notifications
You must be signed in to change notification settings - Fork 450
[Tool] Add tool to print fragment in thread value view #1759
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
Conversation
|
👋 Hi! Thank you for contributing to the TileLang project. Please remember to run We appreciate you taking this step! Our team will review your contribution, and we look forward to your awesome work! 🚀 |
📝 WalkthroughWalkthroughAdded a new plotting helper Changes
Sequence DiagramsequenceDiagram
participant Caller
participant plot_fragment_tv
participant Fragment
participant NumPy
participant Matplotlib
participant FileSystem
Caller->>plot_fragment_tv: invoke plotting helper (frag, options)
plot_fragment_tv->>Fragment: query fragment dims / replication
plot_fragment_tv->>NumPy: construct src index grids (cartesian product)
NumPy-->>plot_fragment_tv: src_flat_idx / src_idx arrays
plot_fragment_tv->>Fragment: map_forward_thread / map_forward_index for each src idx
Fragment-->>plot_fragment_tv: (thread, local) coordinates
plot_fragment_tv->>NumPy: assemble label matrix and color mapping
plot_fragment_tv->>Matplotlib: render annotated grid (figure/axes)
Matplotlib-->>plot_fragment_tv: figure
plot_fragment_tv->>FileSystem: create output dir (if needed) and save file(s)
FileSystem-->>Caller: written image(s)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@tilelang/tools/plot_layout.py`:
- Around line 234-235: The docstring for the parameter save_directory is
incorrect (says default "./tmp" while the function signature uses None); update
the docstring in plot_layout.py to reflect the actual default of save_directory
(None) or change the function signature to match the documented default—modify
the parameter description for save_directory in the relevant function/method's
docstring so it accurately states "default is None" (or adjust the default in
the signature to "./tmp" if that was intended) and ensure the text mentions the
actual behavior used by the code.
- Around line 280-285: The loop can divide by zero when mx =
np.max(src_flat_idx); modify the normalization before calling cmap: compute a
safe denominator (e.g., denom = mx if mx > 0 else 1) and use
cmap(src_flat_idx[i, j] / denom) so values are well-defined when src_flat_idx is
all zeros; update the code around variables mx, src_flat_idx, cmap, and the loop
over num_local_dim/num_thread_dim that calls plt.text with src_idx_str
accordingly.
- Around line 266-275: The loop-local variable "name" in plot_layout.py shadows
the function parameter "name" (the output filename), causing the filename to be
overwritten; rename the loop variable to something non-conflicting (e.g.,
"idx_name" or "idx_label") in both occurrences inside the two loops where it's
assigned (the list comprehension building the index string and the identical
code in the else branch), and update any subsequent uses (src_idx_str[...]=...)
to use the new variable so the function parameter "name" remains the filename.
- Around line 290-297: The figure created by this function is never closed when
save_directory is provided, which can leak memory; after the loop that saves
formats (the block using save_directory, save_dir, formats and
plt.tight_layout()), call plt.close() (or plt.close(fig) if a Figure object
named fig is available) to release the figure, or alternatively return the
Figure object so the caller can close it; update the save block in
plot_layout.py to close the figure after saving.
🧹 Nitpick comments (4)
tilelang/tools/plot_layout.py (4)
3-3: Redundant import ofitertoolsat line 49.The module-level import here makes the local
import itertoolsinsideplot_layout(line 49) redundant. Consider removing the local import for consistency.
283-283: Unused variableafrom tuple unpacking.The alpha channel
ais unpacked but never used.♻️ Proposed fix: prefix with underscore
- r, g, b, a = cmap(src_flat_idx[i, j] / mx) + r, g, b, _a = cmap(src_flat_idx[i, j] / mx)
264-265: Prefer spread syntax over list concatenation.Per static analysis (RUF005),
[rep, *list(item)]is cleaner than[rep] + list(item).♻️ Proposed fix
- th = frag.map_forward_thread([rep] + list(item))[0].value - dst_idx = frag.map_forward_index([rep] + list(item))[0].value + th = frag.map_forward_thread([rep, *item])[0].value + dst_idx = frag.map_forward_index([rep, *item])[0].value
295-296: Inconsistentformatshandling compared toplot_layout.The existing
plot_layoutfunction supports"all"and comma-separated format strings (e.g.,"png,pdf"), while this function only handles plain strings or lists. Consider aligning the behavior for consistency.
This pr add a tool to plot fragment layout in thread and index view.
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.