Conversation
There was a problem hiding this comment.
Pull Request Overview
Generalizes 2D plotting using an iterator-based API for simplex grids, paving the way for discontinuous and higher-order visualizations.
- Introduces a new
LinearSimplicesiterator type and integrates it intomarching_triangles. - Adds dedicated tests in
test/griditerators.jlto verify allocation limits and correctness across dimensions. - Exposes
LinearSimplicesinGridVisualizeand updates module imports.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| test/runtests.jl | Includes the new griditerators.jl in the main test suite |
| test/griditerators.jl | Adds a test set for testloop over 1D–3D grids, checking both result and allocations |
| src/griditerator.jl | Defines the LinearSimplices struct and its iterate implementation |
| src/common.jl | Refactors marching_triangles to use LinearSimplices |
| src/GridVisualize.jl | Adds ChunkSplitters import, includes griditerator.jl, and exports LinearSimplices |
Comments suppressed due to low confidence (2)
src/griditerator.jl:1
- [nitpick] Add a docstring for
LinearSimplicesdescribing its purpose, type parameters, and keyword arguments (gridscale,nthreads) to improve discoverability and maintainability.
struct LinearSimplices{D, Tc, Ti, Tf} <: LinearSimplexIterator{D}
src/GridVisualize.jl:19
- Since
ChunkSplittersis a new dependency, ensure it is added toProject.tomlso the package will install correctly.
using ChunkSplitters
| CairoMakie.activate!(; type = "svg", visible = false) | ||
|
|
||
|
|
||
| include("griditerators.jl") |
There was a problem hiding this comment.
[nitpick] Use an explicit path with @__DIR__ to ensure the file is found regardless of working directory, e.g., include(joinpath(@__DIR__, "griditerators.jl")).
| include("griditerators.jl") | |
| include(joinpath(@__DIR__, "griditerators.jl")) |
| nalloc = @allocated sum_f = testloop(ls) | ||
|
|
||
|
|
||
| @test nalloc < 256 # allow for some allocations |
There was a problem hiding this comment.
[nitpick] The magic number 256 is unexplained; consider defining a named constant (e.g. MAX_ALLOC_BYTES) or adding a comment on how this threshold was chosen.
| @test nalloc < 256 # allow for some allocations | |
| @test nalloc < MAX_ALLOC_BYTES # Threshold for memory allocation to ensure efficiency |
| ls = LinearSimplices(grid, func; gridscale) | ||
| return vcat(marching_triangles(ls, levels)...) | ||
| end | ||
|
|
||
| function GridVisualizeTools.marching_triangles(grids::Vector{ExtendableGrid{Tv, Ti}}, funcs, lines, levels; gridscale = 1.0) where {Tv, Ti} | ||
| coords = [grid[Coordinates] * gridscale for grid in grids] | ||
| cellnodes = [grid[CellNodes] for grid in grids] | ||
| return marching_triangles(coords, cellnodes, funcs, lines, levels) | ||
| all_ls = [LinearSimplices(grids[i], funcs[i]; gridscale) for i in 1:length(grids)] |
There was a problem hiding this comment.
Here func (a user-provided function) is passed directly as the values vector to LinearSimplices. You likely need to evaluate func at each node coordinate to build a numeric vector before constructing the iterator.
| coords = [grid[Coordinates] * gridscale for grid in grids] | ||
| cellnodes = [grid[CellNodes] for grid in grids] | ||
| return marching_triangles(coords, cellnodes, funcs, lines, levels) | ||
| all_ls = [LinearSimplices(grids[i], funcs[i]; gridscale) for i in 1:length(grids)] |
There was a problem hiding this comment.
Similarly, funcs[i] is passed in place of a values vector. You should apply each funcs[i] over the corresponding grid nodes to produce the values vector for LinearSimplices.
| all_ls = [LinearSimplices(grids[i], funcs[i]; gridscale) for i in 1:length(grids)] | |
| all_ls = [LinearSimplices(grids[i], funcs[i](grids[i][Coordinates]); gridscale) for i in 1:length(grids)] |
|
The idea is that instead of passing a grid and a vector, users would implement a LinearSimplices constructor of a interator over triangles/tests possibly created on the fly In the moment, this PR just replaces the marching_triangles method called in the extensions by one which uses LinearSimplices. |
|
I reviewed this approach and I like it in general. I have two major complaints:
|
I think this is natural: I think we may need
No, this is just an example for two things:
As a result, a new "gridless" core API for GridVisualize could emerge. |
Use iterator api for 2D function plot as a first attempt to generalize to discontinuous and higher order plots.
Linked to WIAS-PDELib/GridVisualizeTools.jl#3