Refactoring of canvas #16
Merged
Conversation
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
Contributor
Reviewer's GuideThis PR streamlines the Canvas API by overhauling subplot management, introducing direct line-adding and rendering methods, cleaning up function signatures, exposing Canvas at the package root, and updating tutorials to reflect the new workflow. Class diagram for refactored Canvas and LinePlot classesclassDiagram
class Canvas {
- _subplots: dict
- _num_subplots: int
- _subplot_matrix: list
+ subplots: dict (property)
+ layers: list (property)
+ add_line(x_data, y_data, layer, subplot, row, col, plot_type, **kwargs)
+ add_tikzfigure(col, row, label, **kwargs)
+ add_subplot(col, row, figsize, title, caption, description, label, grid, legend, xmin, xmax, ymin, ymax, xlabel, ylabel, xscale, yscale, xshift, yshift)
+ savefig(filename, layer_by_layer, verbose)
+ plot(backend, savefig, layers)
+ show(backend)
+ plot_matplotlib(savefig, layers, usetex)
+ plot_plotly(savefig)
}
class LinePlot {
- _title: str
- _grid: bool
- _legend: bool
- _xmin: float|int
- _xmax: float|int
- _ymin: float|int
- _ymax: float|int
- _xlabel: str
- _ylabel: str
- _xscale: float|int
- _yscale: float|int
- _xshift: float|int
- _yshift: float|int
- line_data: list
- layered_line_data: dict
- nodes: list
- paths: list
- _node_counter: int
+ add_caption(caption)
+ add_line(x_data, y_data, layer, plot_type, **kwargs)
+ plot_matplotlib(ax, layers)
+ plot_plotly()
}
Canvas "1" o-- "*" LinePlot : contains subplots
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/maxplotlib/canvas/canvas.py:73-81` </location>
<code_context>
assert col is not None, "Not enough columns!"
return row, col
+ def add_line(
+ self,
+ x_data,
+ y_data,
+ layer=0,
+ subplot: LinePlot | None = None,
+ row: int | None = None,
+ col: int | None = None,
+ plot_type="plot",
+ **kwargs,
+ ):
</code_context>
<issue_to_address>
**issue (bug_risk):** The add_line method's error handling for subplot indexing may not catch IndexError.
The try/except block should catch IndexError instead of KeyError, or validate row and col before accessing self._subplot_matrix.
</issue_to_address>
### Comment 2
<location> `src/maxplotlib/canvas/canvas.py:209-212` </location>
<code_context>
+ else:
+ raise ValueError("Invalid backend")
+
+ def show(self, backend="matplotlib"):
if backend == "matplotlib":
- return self.plot_matplotlib(show=show, savefig=savefig, layers=layers)
+ self.plot(backend="matplotlib", savefig=False, layers=None)
+ self._matplotlib_fig.show()
elif backend == "plotly":
- self.plot_plotly(show=show, savefig=savefig)
</code_context>
<issue_to_address>
**issue (bug_risk):** The show method for the plotly backend does not display the figure.
For the plotly backend, add plot.show() to display the figure, matching the behavior of the matplotlib backend.
</issue_to_address>
### Comment 3
<location> `src/maxplotlib/canvas/canvas.py:145-152` </location>
<code_context>
return tikz_figure
- def add_subplot(self, **kwargs):
+ def add_subplot(
+ self,
+ col: int | None = None,
</code_context>
<issue_to_address>
**nitpick:** add_subplot now passes col, row, and label explicitly to LinePlot.
Document or enforce which values take precedence when col, row, or label are provided both as explicit arguments and in kwargs.
</issue_to_address>
### Comment 4
<location> `src/maxplotlib/canvas/canvas.py:87-88` </location>
<code_context>
def add_line(
self,
x_data,
y_data,
layer=0,
subplot: LinePlot | None = None,
row: int | None = None,
col: int | None = None,
plot_type="plot",
**kwargs,
):
if row is not None and col is not None:
try:
subplot = self._subplot_matrix[row][col]
except KeyError:
raise ValueError("Invalid subplot position.")
else:
row, col = 0, 0
subplot = self._subplot_matrix[row][col]
if subplot is None:
row, col = self.generate_new_rowcol(row, col)
subplot = self.add_subplot(col=col, row=row)
subplot.add_line(
x_data=x_data,
y_data=y_data,
layer=layer,
plot_type=plot_type,
**kwargs,
)
</code_context>
<issue_to_address>
**suggestion (code-quality):** Explicitly raise from a previous error ([`raise-from-previous-error`](https://docs.sourcery.ai/Reference/Default-Rules/suggestions/raise-from-previous-error/))
```suggestion
except KeyError as e:
raise ValueError("Invalid subplot position.") from e
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Owner
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/maxplotlib/canvas/canvas.py:85-88` </location>
<code_context>
return row, col
- def add_tikzfigure(self, **kwargs):
+ def add_line(
+ self,
+ x_data,
+ y_data,
+ layer=0,
+ subplot: LinePlot | None = None,
+ row: int | None = None,
+ col: int | None = None,
+ plot_type="plot",
+ **kwargs,
+ ):
+ if row is not None and col is not None:
</code_context>
<issue_to_address>
**suggestion:** Consider handling IndexError instead of KeyError for matrix access.
Catching IndexError will ensure that out-of-bounds access to self._subplot_matrix[row][col] is handled correctly, improving error clarity for invalid indices.
```suggestion
try:
subplot = self._subplot_matrix[row][col]
except IndexError:
raise ValueError("Invalid subplot position: row or column index out of bounds.")
```
</issue_to_address>
### Comment 2
<location> `src/maxplotlib/canvas/canvas.py:246-247` </location>
<code_context>
print(f"Saved {full_filepath}")
- def plot(self, backend="matplotlib", show=True, savefig=False, layers=None):
+ def plot(self, backend="matplotlib", savefig=False, layers=None):
if backend == "matplotlib":
- return self.plot_matplotlib(show=show, savefig=savefig, layers=layers)
+ return self.plot_matplotlib(savefig=savefig, layers=layers)
elif backend == "plotly":
- self.plot_plotly(show=show, savefig=savefig)
+ return self.plot_plotly(savefig=savefig)
+ else:
+ raise ValueError("Invalid backend")
</code_context>
<issue_to_address>
**suggestion:** Raise ValueError for unsupported backend in plot method.
Consider including the backend value in the ValueError message to aid debugging.
```suggestion
+ else:
+ raise ValueError(f"Invalid backend: {backend}")
```
</issue_to_address>
### Comment 3
<location> `src/maxplotlib/canvas/canvas.py:249-256` </location>
<code_context>
+ raise ValueError("Invalid backend")
- def plot_matplotlib(self, show=True, savefig=False, layers=None, usetex=False):
+ def show(self, backend="matplotlib"):
+ if backend == "matplotlib":
+ self.plot(backend="matplotlib", savefig=False, layers=None)
+ self._matplotlib_fig.show()
+ elif backend == "plotly":
+ plot = self.plot_plotly(savefig=False)
+ else:
+ raise ValueError("Invalid backend")
+
</code_context>
<issue_to_address>
**suggestion:** show() method does not return the plotly figure.
For the plotly backend, please return the figure or call fig.show() to align with the matplotlib behavior.
```suggestion
def show(self, backend="matplotlib"):
if backend == "matplotlib":
self.plot(backend="matplotlib", savefig=False, layers=None)
self._matplotlib_fig.show()
elif backend == "plotly":
fig = self.plot_plotly(savefig=False)
fig.show()
return fig
else:
raise ValueError("Invalid backend")
```
</issue_to_address>
### Comment 4
<location> `src/maxplotlib/canvas/canvas.py:87-88` </location>
<code_context>
def add_line(
self,
x_data,
y_data,
layer=0,
subplot: LinePlot | None = None,
row: int | None = None,
col: int | None = None,
plot_type="plot",
**kwargs,
):
if row is not None and col is not None:
try:
subplot = self._subplot_matrix[row][col]
except KeyError:
raise ValueError("Invalid subplot position.")
else:
row, col = 0, 0
subplot = self._subplot_matrix[row][col]
if subplot is None:
row, col = self.generate_new_rowcol(row, col)
subplot = self.add_subplot(col=col, row=row)
subplot.add_line(
x_data=x_data,
y_data=y_data,
layer=layer,
plot_type=plot_type,
**kwargs,
)
</code_context>
<issue_to_address>
**suggestion (code-quality):** Explicitly raise from a previous error ([`raise-from-previous-error`](https://docs.sourcery.ai/Reference/Default-Rules/suggestions/raise-from-previous-error/))
```suggestion
except KeyError as e:
raise ValueError("Invalid subplot position.") from e
```
</issue_to_address>
### Comment 5
<location> `src/maxplotlib/subfigure/line_plot.py:21-29` </location>
<code_context>
def __init__(
self,
nodes,
path_actions=[],
cycle=False,
label="",
layer=0,
**kwargs,
):
self.nodes = nodes
self.path_actions = path_actions
self.cycle = cycle
self.layer = layer
self.label = label
self.options = kwargs
</code_context>
<issue_to_address>
**suggestion (code-quality):** Replace mutable default arguments with None ([`default-mutable-arg`](https://docs.sourcery.ai/Reference/Default-Rules/suggestions/default-mutable-arg/))
```suggestion
def __init__(self, nodes, path_actions=None, cycle=False, label="", layer=0, **kwargs):
if path_actions is None:
path_actions = []
```
</issue_to_address>
### Comment 6
<location> `src/maxplotlib/subfigure/tikz_figure.py:86` </location>
<code_context>
def __init__(
self,
nodes,
path_actions=[],
cycle=False,
label="",
layer=0,
**kwargs,
):
"""
Represents a path (line) connecting multiple nodes.
Parameters:
- nodes (list of str): List of node names to connect.
- **kwargs: Additional TikZ path options (e.g., style, color).
"""
self.nodes = nodes
self.path_actions = path_actions
self.cycle = cycle
self.layer = layer
self.label = label
self.options = kwargs
</code_context>
<issue_to_address>
**issue (code-quality):** Replace mutable default arguments with None ([`default-mutable-arg`](https://docs.sourcery.ai/Reference/Default-Rules/suggestions/default-mutable-arg/))
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
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.
Summary by Sourcery
Refactor Canvas and subfigure APIs for clearer usage, introduce direct line plotting, unify method signatures, and update tutorials accordingly
New Features:
Enhancements:
Documentation: