Conversation
Closed
Contributor
Reviewer's GuideIntroduces a full “gallery” of Jupyter‐based tutorials integrated into Sphinx, updates CI to auto‐build and publish HTML docs, refactors a couple of plotting interfaces, and cleans up the existing tutorial notebooks for consistency. Class diagram for updated plotting interfacesclassDiagram
class Canvas {
+plot_matplotlib(show=True, savefig=False, layers=None, usetex=False)
}
class TikzFigure {
+plot_matplotlib(ax, layers=None)
}
class LinePlot {
+add_node(x, y, label=None, content="", layer=0, **kwargs)
}
Canvas --> TikzFigure : add_tikzfigure()
Canvas --> LinePlot : add_subplot()
TikzFigure <|-- LinePlot
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
There was a problem hiding this comment.
Hey @max-models - I've reviewed your changes - here's some feedback:
- It looks like you left debug
printstatements incanvas.plot_matplotlibandline_plot.add_node—please remove those so the API is clean. - This PR mixes huge notebook-output stripping with API changes and CI additions—consider splitting notebook cleanup, code refactors (like the
plot_matplotlibsignature change), and docs/CI updates into smaller focused PRs.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- It looks like you left debug `print` statements in `canvas.plot_matplotlib` and `line_plot.add_node`—please remove those so the API is clean.
- This PR mixes huge notebook-output stripping with API changes and CI additions—consider splitting notebook cleanup, code refactors (like the `plot_matplotlib` signature change), and docs/CI updates into smaller focused PRs.
## Individual Comments
### Comment 1
<location> `src/maxplotlib/canvas/canvas.py:208` </location>
<code_context>
for (row, col), subplot in self.subplots.items():
ax = axes[row][col]
+ print(f"{subplot = }")
subplot.plot_matplotlib(ax, layers=layers)
# ax.set_title(f"Subplot ({row}, {col})")
</code_context>
<issue_to_address>
Debug print statement left in production code.
Remove the print statement or replace it with appropriate logging if necessary.
</issue_to_address>
### Comment 2
<location> `src/maxplotlib/subfigure/line_plot.py:250` </location>
<code_context>
if layer in self.layers:
self.layers[layer].add(node)
else:
+ print(f"{self.layers = } {layer = }")
self.layers[layer] = Tikzlayer(layer)
self.layers[layer].add(node)
</code_context>
<issue_to_address>
Debug print statement should be removed.
Consider removing this print statement or replacing it with a logger at the appropriate log level.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Published docs to https://max-models.github.io/maxplotlib/
For now, the tutorials are simply generated