Open
Conversation
dual(primal::CellComplex{N, K}, center::Function) where {N, K}
Compute the dual TriangulatedComplex of a simplicial complex. `center` is a function that
takes a `Simplex{N, K}` to a `Barycentric{N, K}`.
Returns:
- `dual_tcomp`: The dual TriangulatedComplex.
- `primal_to_duals`: A dictionary mapping each primal Cell to its corresponding dual Cell.
- Cell(s::Simplex{N, K}) where {N, K} = Cell(s.points, K)
+ Cell(s::Simplex{N, K}) where {N, K} = Cell(s.points, K - 1)
+ Cell(s::Simplex{N, K}) where {N, K} = Cell(s.points, K)
- Cell(s::Simplex{N, K}) where {N, K} = Cell(s.points, K - 1)
- Add barycentric_hodge function using barycentric dual construction - Add corrected_barycentric_hodge with direct gradient and cross diffusion corrections - Implement direct_gradient_correction to address non-orthogonality issues - Implement cross_diffusion_correction to reduce numerical diffusion - Add comprehensive test suite for new functionality - Export new functions from operators module This addresses numerical errors in discrete exterior calculus when using barycentric centers instead of circumcenters, providing better stability while correcting for lack of orthogonality. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Replace Dict-based center cache with Vector + index lookup - Optimize Point(SimpleBarycentric) to avoid intermediate objects - Replace map with manual loops in SimpleSimplex creation - Use Set for point deduplication instead of unique(vcat(...)) - Add sizehint! for Dict pre-allocation - Parallelize Phase0 (center computation), Phase1 (elementary duals), Phase2 (dual cell creation) Performance improvement: DEC.Mesh from 48s to 12s (74% reduction)
- Phase 0: Parallel center precomputation with Vector storage - Phase 1: Sequential elementary_duals with cached centers (NOT parallel) - Phase 2: Parallel dual cell creation with Set-based point deduplication - Point(SimpleBarycentric): Direct computation avoiding intermediate objects Previous parallel Phase 1 caused race conditions in dictionary access. This version maintains 70% speedup (48s -> 14s) with correct results.
- Replace recursive Dict-based elementary_duals with non-recursive vector-based version - Parallelize Phase 1: cells of same dimension are independent - Store results in Vector instead of Dict for faster access Performance: DEC.Mesh 14.5s -> 12.7s (13% faster)
- parent!を使わず、parents/childrenを2パスでセル単位に構築 - dual_of_idx/signed_duals_of_idxに集約してDict/push!競合を回避 - 大規模メッシュでのスレッドスケール改善を狙う
![windsurf-bot[bot]](https://avatars.githubusercontent.com/in/1066231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Other comments (15)
-
Project.toml (24-24)
The Julia version compatibility specification has incorrect syntax. The range should use a tilde (`~`) or caret (`^`) operator, not a dash. Also, Julia 1.12 doesn't exist yet (current stable is 1.10.x as of January 2026).
julia = "^1.6" - src/operators.jl (343-348) The `corresponds_to_cell` function returns `true` for all cases where k != 1, which seems too permissive. This could lead to incorrect dual cell mappings for higher-dimensional forms. Consider implementing more specific correspondence checks for different values of k.
- src/mesh.jl (356-356) The `dual2` function returns a tuple `(dual_tcomp, primal_to_duals)` while the original `dual` function only returns `dual_tcomp`. This API inconsistency could cause errors if code that expects just the complex tries to use `dual2`. Consider either making both functions return the same type or clearly documenting this difference.
- test/test_operators.jl (193-193) Consider adding an `@test length(comp.cells[k]) > 1` before this conditional block to ensure the test doesn't silently skip the helper function tests. This would make it explicit when these tests aren't being run.
- test/test_operators.jl (223-223) Consider adding an assertion that the matrices should have non-zero size to ensure this test is meaningful. Otherwise, the important comparison test might be silently skipped.
- test/test_simplices.jl (32-32) I notice you've increased the tolerance in this test from 1e-5 to 2e-5. Is this addressing a specific test failure? It might be worth adding a comment explaining why this change was necessary, especially if it's related to numerical precision issues in certain calculations.
- test_barycentric_hodge.jl (22-28) This test only checks if functions are defined in the Main module, but doesn't actually import the module being tested or call the functions with test data. Consider importing the actual module and adding functional tests that verify the mathematical correctness of the implementations.
- test_barycentric_hodge.jl (44-45) The mock cell objects are created but never used in any tests. Consider either removing them or implementing actual tests that use these objects to verify the behavior of the hodge operators with realistic mesh structures.
-
Project.toml (46-46)
Documenter is included in the [extras] section but the corresponding docs target is commented out. If you're using Documenter, you should uncomment the docs target line.
docs = ["Documenter"] - src/mesh.jl (188-196) There's a potential thread safety issue in the Phase 2 parallel computation. The `push!(points_set, pt)` operation inside the nested loops is not thread-safe when executed in parallel. However, since this is inside a single thread's execution context (each thread has its own `points_set`), it's actually safe. Consider adding a comment to clarify this is intentionally thread-local to avoid confusion.
- src/mesh.jl (87-87) Consider translating the Japanese comments to English for better accessibility in this open-source project. For example, the comment "非再帰版: 単一セルのelementary dualを計算（親は既に計算済み前提）" could be translated to "Non-recursive version: Calculate elementary dual of a single cell (assuming parents are already calculated)".
- src/simplices.jl (18-18) The implementation uses `zeros(Float64, N)` which creates a mutable array, but the Point constructor expects an SVector. This could cause an unnecessary allocation and conversion. Consider using `SVector{N, Float64}` directly or ensuring the result is properly converted to match the Point constructor's expectations.
- test_barycentric_hodge.jl (31-31) The file claims to test mathematical properties (line 31) but doesn't actually verify any mathematical properties of the hodge operators, such as symmetry, positive-definiteness, or consistency with exterior calculus identities. Consider adding tests that verify these important properties.
- Manifest.toml (7-7) There are non-English comments in the file (e.g., line 7: "バージョンを0.2.0に更新" and line 34: "Compat 2.2.0を追加"). Consider using English for comments to ensure all contributors can understand them.
- src/operators.jl (356-364) The `estimate_orthogonality_deviation` function uses hardcoded values (0.2, 0.15, 0.1) instead of computing actual geometric relationships between cells. This could lead to inaccurate corrections. Consider implementing a geometric calculation based on the actual cell geometries.
💡 To request another review, post a new comment with "/windsurf-review".
| deps = ["Base64", "Dates", "DelimitedFiles", "Distributed", "InteractiveUtils", "LibGit2", "Libdl", "LinearAlgebra", "Markdown", "Mmap", "Pkg", "Printf", "REPL", "Random", "Serialization", "SharedArrays", "Sockets", "SparseArrays", "Statistics", "Test", "UUIDs", "Unicode"] | ||
| git-tree-sha1 = "ed2c4abadf84c53d9e58510b5fc48912c2336fbb" | ||
| uuid = "34da2185-b29b-5c13-b0c7-acf172513d20" | ||
| # version = "2.2.0" # Compat 2.2.0を追加 |
There was a problem hiding this comment.
The version numbers for Compat and UniqueVectors packages are commented out. This can cause dependency resolution issues. Either uncomment these version numbers or remove them completely if you want to use the latest versions.
Comment on lines
+278
to
+279
| correction_strength = 0.1 | ||
| return correction_strength * orthogonality_deviation * sqrt(p_vol * d_vol) / (p_vol + d_vol) |
There was a problem hiding this comment.
The sqrt function is used in compute_direct_gradient_weight but is not imported. You need to add using Base: sqrt at the top of the file.
| uuid = "0a3a9624-8642-11e9-0f42-fd2a0d40ec72" | ||
| authors = ["Michael Scheer <mgscheer@gmail.com>"] | ||
| version = "0.1.0" | ||
| authors = ["Michael Scheer <mgscheer@gmail.com>", "H-Fukagawa <あなたのEmailなど>"] # 必要に応じて更新 |
There was a problem hiding this comment.
There's a placeholder in the authors field (あなたのEmailなど which means 'your email, etc.' in Japanese). This should be replaced with actual contact information before merging.
This reverts commit 3c578ca.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.