LIHPC-Computational-Geometry / honeycomb

Combinatorial map implementation for meshing applications
https://lihpc-computational-geometry.github.io/honeycomb/
Apache License 2.0
7 stars 1 forks source link

refactor!(core): rework `sew` methods to allow for more control #224

Open imrn99 opened 2 days ago

imrn99 commented 2 days ago

I split the original link_and_sew.rs file into two modules: links and sews, each having submodules for dim1 and dim2.

there are now 3 and 2 variants for (un)sew and (un)link methods respectively. Here are two tables summarizing their differences (for sew and link, complementary methods follow the same pattern):

sew variant description
try_<DIM>_sew defensive impl, only succeding if attributes are correctly merged & the transaction is validated
<DIM>_sew regular impl, which uses attribute fallback policies and will fail only if the transaction fails
force_<DIM>_sew convenience impl, which follows the same logic as the regular impl, but using a transaction internally to retry until success. useful when precise control isn't needed (e.g. in sequential impls)
link variant description
<DIM>_link regular impl, which will fail if the transaction fails
force_<DIM>_sew convenience impl, which follows the same logic as the regular impl, but using a transaction internally to retry until success. useful when precise control isn't needed (e.g. in sequential impls)

TODO: implement the try_ variants. I want to do it in a separate PR because I will modify the attribute storage traits to follow a similar pattern. I'm guessing do it here would double the size of the PR, and unnecessarily mix unrelated changes

codecov-commenter commented 2 days ago

Codecov Report

Attention: Patch coverage is 62.32073% with 289 lines in your changes missing coverage. Please review.

Project coverage is 80.67%. Comparing base (ac36dbf) to head (f1b1135).

Files with missing lines Patch % Lines
honeycomb-core/src/cmap/dim2/sews/two.rs 43.24% 210 Missing :warning:
honeycomb-core/src/cmap/dim2/sews/one.rs 53.33% 42 Missing :warning:
honeycomb-core/src/cmap/dim2/links/one.rs 28.57% 15 Missing :warning:
honeycomb-core/src/cmap/dim2/links/two.rs 28.57% 15 Missing :warning:
honeycomb-kernels/src/triangulation/fan.rs 50.00% 7 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #224 +/- ## ========================================== - Coverage 82.26% 80.67% -1.60% ========================================== Files 48 51 +3 Lines 6789 6850 +61 ========================================== - Hits 5585 5526 -59 - Misses 1204 1324 +120 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

imrn99 commented 2 days ago

An a priori comment: I am not convinced by using Result for the transactions. Do you have a strong argument for using this instead of Option ?

I'm not sure to exactly which Result you refer to. The stm crate originally uses results to pass value through (see StmResult), so I aligned the API with this.

In our case, we could switch the Result to an Option for all instances of Result<(), _>, if that's what you mean

cedricchevalier19 commented 2 days ago

No, do not change, I was wondering the reason. But mimicking the stm crate is a pretty valid one.