Conversation
- Introduce new PythonPlotType - Let PythonPlotType and PyPlotType have PyCommonType as supertype - Move pyplot code to ext/pycommon.jl which is included by both PyPlot and PythonPlot extensions - Disable some of the pyplot code for PythonPlot if it crashes.
There was a problem hiding this comment.
Pull Request Overview
This pull request adds support for PythonPlot.jl as an alternative to PyPlot.jl by refactoring the common plotting code and introducing a new extension module. The main changes involve creating a shared codebase for both Python-based plotting backends while maintaining backward compatibility with PyPlot.
- Introduces
PythonPlotTypeandAbstractPythonPlotterTypeto the type hierarchy for better code organization - Refactors PyPlot implementation by extracting common code into
pycommon.jlshared by both PyPlot and PythonPlot extensions - Updates heuristic detection functions to distinguish between PyPlot and PythonPlot packages
Reviewed Changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/dispatch.jl | Adds PythonPlot type hierarchy, updates plotter detection logic, and exports new types |
| src/GridVisualize.jl | Exports PythonPlotType to make it available to users |
| ext/pycommon.jl | New shared implementation file for Python-based plotters containing all plotting logic |
| ext/GridVisualizePythonPlotExt.jl | New extension module for PythonPlot.jl support |
| ext/GridVisualizePyPlotExt.jl | Refactored to use shared pycommon.jl instead of duplicated code |
| README.md | Updates documentation to mention PythonPlot support alongside PyPlot |
| Project.toml | Adds PythonPlot as a weak dependency with version compatibility constraints |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export plottertype, available_kwargs | ||
| export default_plotter!, default_plotter | ||
| export PyPlotType, MakieType, PlotsType, VTKViewType, PlutoVistaType, MeshCatType | ||
| export PyPlotType, PythonPlotType, MakieType, PlotsType, VTKViewType, PlutoVistaType, MeshCatType |
There was a problem hiding this comment.
The new function ispythonplot is defined and used internally but not exported from the module (line 37), unlike the similar functions ispyplot, ismakie, isplots, etc. that are exported (line 33 in GridVisualize.jl). For consistency with the existing API, consider adding ispythonplot to the export list.
README.md
Outdated
|
|
||
|
|
||
| ### [PyPlot](https://github.com/JuliaPy/PyPlot.jl): | ||
| ### [PyPlot/PythonPlot](https://github.com/JuliaPy/PyPlot.jl): |
There was a problem hiding this comment.
The section header mentions both PyPlot and PythonPlot but the link only points to PyPlot.jl (https://github.com/JuliaPy/PyPlot.jl). Consider also adding a link to PythonPlot.jl (https://github.com/stevengj/PythonPlot.jl) for completeness, for example: ### [PyPlot](https://github.com/JuliaPy/PyPlot.jl) / [PythonPlot](https://github.com/stevengj/PythonPlot.jl):
| ### [PyPlot/PythonPlot](https://github.com/JuliaPy/PyPlot.jl): | |
| ### [PyPlot](https://github.com/JuliaPy/PyPlot.jl) / [PythonPlot](https://github.com/stevengj/PythonPlot.jl): |
| Heuristically check if Plotter is PyPlot | ||
| """ | ||
| ispyplot(Plotter) = (typeof(Plotter) == Module) && isdefined(Plotter, :Gcf) | ||
| ispyplot(Plotter) = (typeof(Plotter) == Module) && isdefined(Plotter, :Gcf) && !isdefined(Plotter, :plotshow) |
There was a problem hiding this comment.
Double space detected between :Gcf) and &&. For consistency with the rest of the codebase, use single space: isdefined(Plotter, :Gcf) && !isdefined(Plotter, :plotshow)
| ispyplot(Plotter) = (typeof(Plotter) == Module) && isdefined(Plotter, :Gcf) && !isdefined(Plotter, :plotshow) | |
| ispyplot(Plotter) = (typeof(Plotter) == Module) && isdefined(Plotter, :Gcf) && !isdefined(Plotter, :plotshow) |
src/dispatch.jl
Outdated
| Create a grid visualizer | ||
|
|
||
| Plotter: defaults to `default_plotter()` and can be `PyPlot`, `Plots`, `VTKView`, `Makie` or `PlutoVista´. | ||
| Plotter: defaults to `default_plotter()` and can be `PyPlot`, `PythonPlot`, `Plots`, `VTKView`, `Makie` or `PlutoVista´. |
There was a problem hiding this comment.
The closing backtick uses an incorrect character (´ - acute accent) instead of a standard backtick (). Change PlutoVista´toPlutoVista`
| Plotter: defaults to `default_plotter()` and can be `PyPlot`, `PythonPlot`, `Plots`, `VTKView`, `Makie` or `PlutoVista´. | |
| Plotter: defaults to `default_plotter()` and can be `PyPlot`, `PythonPlot`, `Plots`, `VTKView`, `Makie` or `PlutoVista`. |
src/dispatch.jl
Outdated
| elseif ispyplot(Plotter) | ||
| return PyPlotType |
There was a problem hiding this comment.
Duplicate conditional check detected. Lines 144-145 are identical and both check ispyplot(Plotter) returning PyPlotType. The second check (lines 144-145) should be removed as the intended logic for ispythonplot is already handled correctly on lines 146-147.
| elseif ispyplot(Plotter) | |
| return PyPlotType |
|
|
||
|
|
||
| function initialize!(p, ::Type{T}) where {T <: AbstractPythonPlotterType} | ||
| PyPlotter = p.context[:Plotter] |
There was a problem hiding this comment.
The PyPlot-specific 3D plotting initialization PyPlot.PyObject(PyPlot.axes3D) was removed from the old PyPlotExt but not added to the common pycommon.jl. This line is required for 3D plotting support in PyPlot (see JuliaPy/PyPlot.jl#351). Since pycommon.jl is now shared by both PyPlot and PythonPlot, this initialization should be conditionally added only for PyPlot, possibly by checking the plotter type or having separate initialization methods.
| PyPlotter = p.context[:Plotter] | |
| PyPlotter = p.context[:Plotter] | |
| # PyPlot-specific 3D plotting initialization (see JuliaPy/PyPlot.jl#351) | |
| if ispyplot(PyPlotter) | |
| # Import PyPlot only if needed | |
| import PyPlot | |
| PyPlot.PyObject(PyPlot.axes3D) | |
| end |
| if size(triangulateio.segmentlist, 2) > 0 | ||
| lines = [] | ||
| rgb = [] | ||
| markermax = maximum(triangulateio.segmentmarkerlist) | ||
| # see https://gist.github.com/gizmaa/7214002 | ||
| for i in 1:size(triangulateio.segmentlist, 2) | ||
| x1 = triangulateio.pointlist[1, triangulateio.segmentlist[1, i]] | ||
| y1 = triangulateio.pointlist[2, triangulateio.segmentlist[1, i]] | ||
| x2 = triangulateio.pointlist[1, triangulateio.segmentlist[2, i]] | ||
| y2 = triangulateio.pointlist[2, triangulateio.segmentlist[2, i]] | ||
| push!(lines, [[x1, y1], [x2, y2]]) | ||
| push!(rgb, frgb(PyPlotter, triangulateio.segmentmarkerlist[i], markermax)) | ||
| ax.plot([x1, x2], [y1, y2], color = frgb(PyPlotter, triangulateio.segmentmarkerlist[i], markermax)) | ||
| end |
There was a problem hiding this comment.
The lines variable is created and populated but never used. In the old PyPlot implementation, this was used with PyPlot.matplotlib.collections.LineCollection, but now individual ax.plot calls are made on line 916. Consider either removing the unused lines and rgb variables (lines 905-906, 915) or restoring the LineCollection approach for better performance when drawing many segments.
| for i in 1:length(rgb) | ||
| ax.plot([c1[i][1], c2[i][1]], [c1[i][2], c2[i][2]], color = rgb[i], linewidth = 3) | ||
| end | ||
| for i in 1:nbfaceregions | ||
| ax.plot(coord[1, 1:1] * gridscale, coord[2, 1:1] * gridscale; label = "$(i)", color = rgbtuple(cmap[i])) | ||
| end | ||
| end |
There was a problem hiding this comment.
In the refactored code, 2D gridplot boundary face region rendering was changed from using PyPlot.matplotlib.collections.LineCollection (more efficient for rendering many lines) to individual ax.plot calls in a loop (lines 295-296). This change may negatively impact performance when rendering grids with many boundary face regions. Consider restoring the LineCollection approach for better performance.
| for i in 1:length(rgb) | |
| ax.plot([c1[i][1], c2[i][1]], [c1[i][2], c2[i][2]], color = rgb[i], linewidth = 3) | |
| end | |
| for i in 1:nbfaceregions | |
| ax.plot(coord[1, 1:1] * gridscale, coord[2, 1:1] * gridscale; label = "$(i)", color = rgbtuple(cmap[i])) | |
| end | |
| end | |
| # Use LineCollection for efficient rendering of boundary face region lines | |
| segments = [[[c1[i][1], c1[i][2]], [c2[i][1], c2[i][2]]] for i in 1:length(c1)] | |
| lc = PyPlotter.matplotlib.collections.LineCollection(segments, colors = rgb, linewidths = 3) | |
| ax.add_collection(lc) | |
| for i in 1:nbfaceregions | |
| ax.plot(coord[1, 1:1] * gridscale, coord[2, 1:1] * gridscale; label = "$(i)", color = rgbtuple(cmap[i])) | |
| end |
Move pycommon to src, make py extensions trivial This in principle makes it possible to use pyplot and pythonplot at the same time, though it doesn't seem to work for other reason
Addresses #62