Updated Hodge Star for 1-Forms #35
Conversation
|
There are still a few things that need to be decided before merging this PR, specifically:
|
|
Thanks for implementing this improved Hodge star. Re: your first question, I think we should use the dispatch-on-algorithm design pattern, used elsewhere in this package to select to the subdivision method. We should make this new Hodge star the default, but keep the diagonal Hodge star around for benchmarking and debugging purposes. |
src/DiscreteExteriorCalculus.jl
Outdated
| e = reverse(triangle_edges(s, t)) | ||
| ev = point(s, tgt(s, e)) .- point(s, src(s,e)) | ||
|
|
||
| dv = fill(dual_point(s, triangle_center(s, t)),3) .- dual_point(s, edge_center(s, e)) |
There was a problem hiding this comment.
I think that you can avoid this fill because of the semantics of broadcasting .- either you can avoid it or pull it outside the subtraction.
There was a problem hiding this comment.
Yeah, it'd be nice to remove that fill, but I'm not sure how to get it to broadcast the first argument (which is a 3-element vector) over the second argument (which is a 3-element vector of 3-element vectors). In this last commit I've changed it out with a map call, but I'd be interested in learning about other solutions to this.
|
I've tested the new hodge on some physical systems, and it seems to be performing as expected! I've included tests which should evaluate both the diagonal and geometric hodge stars, and added a keyword system for specifying hodge star and matrix type from any of the methods that depend on these. I'll just be adding some tests for codecov, but the body of the code is ready for another review! |
|
100% coverage now! |
test/DiscreteExteriorCalculus.jl
Outdated
| @@ -6,6 +6,7 @@ using SparseArrays, StaticArrays | |||
|
|
|||
| using Catlab.CategoricalAlgebra.CSets | |||
| using CombinatorialSpaces | |||
| using CombinatorialSpaces.DiscreteExteriorCalculus: inv_hodge_star | |||
There was a problem hiding this comment.
I guess we should go ahead and export this function.
There was a problem hiding this comment.
Would it make sense to also include a function for ⋆ that accepts a DualForm and calls inv_hodge_star?
There was a problem hiding this comment.
I'd prefer to introduce a new operator ⋆⁻¹ as in the exterior calculus GAT.
There was a problem hiding this comment.
Ah, yeah, that makes sense.
|
Thanks Andrew! I will go ahead and merge this now. |
This PR provides an implementation of the hodge star on primal 1-forms presented in Ayoub et al 2021.