-
Notifications
You must be signed in to change notification settings - Fork 7
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
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") |
Copilot
AI
Jun 17, 2025
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")) |
| nalloc = @allocated sum_f = testloop(ls) | ||
|
|
||
|
|
||
| @test nalloc < 256 # allow for some allocations |
Copilot
AI
Jun 17, 2025
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 |
| 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)] |
Copilot
AI
Jun 17, 2025
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.
| 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)] |
Copilot
AI
Jun 17, 2025
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)] |
|
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