Skip to content

Conversation

j-fu
Copy link
Member

@j-fu j-fu commented Jan 25, 2024

No description provided.

@j-fu j-fu marked this pull request as draft November 23, 2024 21:51
Copy link

@Copilot Copilot AI left a 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 LinearSimplex with typed points/values and iterator interface in src/linearsimplex.jl.
  • Add marching_iterators.jl for intersection computations and a threaded marching_triangles function.
  • Refactor src/marching.jl for plane‐tetrahedron intersections and update GridVisualizeTools.jl to 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: calculatCe should be calculate to match the existing naming and ensure callers resolve correctly.
function calculatCe_plane_tetrahedron_intersection!(

src/linearsimplex.jl:1

  • Public type LinearSimplex is missing a docstring. Consider adding documentation to explain its purpose, parameters, and usage.
struct LinearSimplex{D, N, Tv}

Comment on lines +55 to +61
map(triangle_iterators) do triangles
intersections(triangles, levels)
end
else
threads = map(triangle_iterators) do triangles
Threads.@spawn intersections(triangles, levels)
end
Copy link

Copilot AI Jun 17, 2025

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.

Suggested change
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)

Copilot uses AI. Check for mistakes.

Comment on lines +59 to +61
threads = map(triangle_iterators) do triangles
Threads.@spawn intersections(triangles, levels)
end
Copy link

Copilot AI Jun 17, 2025

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)
Suggested change
threads = map(triangle_iterators) do triangles
Threads.@spawn intersections(triangles, levels)
end
threads = map(triangles -> Threads.@spawn(intersections(triangles, levels)), triangle_iterators)

Copilot uses AI. Check for mistakes.

function pushtris(ns, ixcoord, ixvalues)
# number of intersection points can be 3 or 4
if ns >= 3
return if ns >= 3
Copy link

Copilot AI Jun 17, 2025

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.

Suggested change
return if ns >= 3
return if ns < 3

Copilot uses AI. Check for mistakes.

using DocStringExtensions: SIGNATURES, TYPEDEF, TYPEDSIGNATURES
using StaticArraysCore: SVector
using StaticArrays: @MArray
using GeometryBasics # replace by SArray!
Copy link

Copilot AI Jun 17, 2025

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.

Suggested change
using GeometryBasics # replace by SArray!
using StaticArrays: SArray

Copilot uses AI. Check for mistakes.

j-fu and others added 3 commits June 17, 2025 14:25
Moved LinearSimplex.jl to experimental folder
Created GridSimplex.jl in experimental folder
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants