-
Notifications
You must be signed in to change notification settings - Fork 4
Iterator api #3
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 #3
Conversation
Replace this later by SArray.
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
This PR introduces a new iterator‐based API for marching simplices, adds a core LinearSimplex type with constructors, and wires up threading support in the marching routines.
- Define
LinearSimplexwith typed points/values and iterator interface insrc/linearsimplex.jl. - Add
marching_iterators.jlfor intersection computations and a threadedmarching_trianglesfunction. - Refactor
src/marching.jlfor plane‐tetrahedron intersections and updateGridVisualizeTools.jlto include the new modules.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/linearsimplex.jl | Add LinearSimplex struct, constructors, and iterator utilities |
| src/marching_iterators.jl | Implement pushintersection!, intersections, and threaded mapping |
| src/marching.jl | Rename and adjust plane‐tetrahedron intersection and pushtris logic |
| src/GridVisualizeTools.jl | Import/export new modules (linearsimplex, marching_iterators) |
| src/colors.jl | Inject $(SIGNATURES) into an existing docstring |
Comments suppressed due to low confidence (2)
src/marching.jl:64
- Typo in function name:
calculatCeshould becalculateto match the existing naming and ensure callers resolve correctly.
function calculatCe_plane_tetrahedron_intersection!(
src/linearsimplex.jl:1
- Public type
LinearSimplexis missing a docstring. Consider adding documentation to explain its purpose, parameters, and usage.
struct LinearSimplex{D, N, Tv}
| map(triangle_iterators) do triangles | ||
| intersections(triangles, levels) | ||
| end | ||
| else | ||
| threads = map(triangle_iterators) do triangles | ||
| Threads.@spawn intersections(triangles, levels) | ||
| end |
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.
The call to map uses the collection as the first argument and a do-block as the second; map expects a function first and collection(s) after. Consider using map(triangles -> intersections(triangles, levels), triangle_iterators) or swapping the arguments appropriately.
| map(triangle_iterators) do triangles | |
| intersections(triangles, levels) | |
| end | |
| else | |
| threads = map(triangle_iterators) do triangles | |
| Threads.@spawn intersections(triangles, levels) | |
| end | |
| map(triangles -> intersections(triangles, levels), triangle_iterators) | |
| else | |
| threads = map(triangles -> Threads.@spawn intersections(triangles, levels), triangle_iterators) |
| threads = map(triangle_iterators) do triangles | ||
| Threads.@spawn intersections(triangles, levels) | ||
| end |
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.
Same issue with argument ordering in map for spawning threads; the function should be the first argument. Consider:
threads = map(triangles -> Threads.@spawn intersections(triangles, levels), triangle_iterators)| threads = map(triangle_iterators) do triangles | |
| Threads.@spawn intersections(triangles, levels) | |
| end | |
| threads = map(triangles -> Threads.@spawn(intersections(triangles, levels)), triangle_iterators) |
| function pushtris(ns, ixcoord, ixvalues) | ||
| # number of intersection points can be 3 or 4 | ||
| if ns >= 3 | ||
| return if ns >= 3 |
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.
This early return inverts the original logic by skipping valid cases when ns >= 3. It likely should be return if ns < 3 to skip cases with fewer than three intersections.
| return if ns >= 3 | |
| return if ns < 3 |
| using DocStringExtensions: SIGNATURES, TYPEDEF, TYPEDSIGNATURES | ||
| using StaticArraysCore: SVector | ||
| using StaticArrays: @MArray | ||
| using GeometryBasics # replace by SArray! |
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] There's a TODO-style comment suggesting a future replacement of this import. Consider addressing or removing this to avoid lingering technical debt.
| using GeometryBasics # replace by SArray! | |
| using StaticArrays: SArray |
Moved LinearSimplex.jl to experimental folder Created GridSimplex.jl in experimental folder
No description provided.