-
Notifications
You must be signed in to change notification settings - Fork 8
Iterator api #23
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
base: main
Are you sure you want to change the base?
Iterator api #23
Conversation
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.
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
LinearSimplices
iterator type and integrates it intomarching_triangles
. - Adds dedicated tests in
test/griditerators.jl
to verify allocation limits and correctness across dimensions. - Exposes
LinearSimplices
inGridVisualize
and 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
LinearSimplices
describing 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
ChunkSplitters
is a new dependency, ensure it is added toProject.toml
so the package will install correctly.
using ChunkSplitters
@@ -4,6 +4,9 @@ import CairoMakie, PyPlot, PlutoVista | |||
|
|||
CairoMakie.activate!(; type = "svg", visible = false) | |||
|
|||
|
|||
include("griditerators.jl") |
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.
[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")) |
Copilot uses AI. Check for mistakes.
nalloc = @allocated sum_f = testloop(ls) | ||
|
||
|
||
@test nalloc < 256 # allow for some allocations |
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.
[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 |
Copilot uses AI. Check for mistakes.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Copilot uses AI. Check for mistakes.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)] |
Copilot uses AI. Check for mistakes.
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