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): change `split_edge` & `splitn_edge` to use preallocated darts #180

Closed imrn99 closed 1 month ago

imrn99 commented 1 month ago

both methods now take a slice of dart IDs to use as new segments components. both methods check if:

Scope

Type of change

Other

codecov-commenter commented 1 month ago

Codecov Report

Attention: Patch coverage is 96.51899% with 22 lines in your changes missing coverage. Please review.

Project coverage is 82.61%. Comparing base (798c5e8) to head (ad78351). Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
honeycomb-core/src/cmap/dim2/advanced_ops.rs 89.76% 22 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #180 +/- ## ========================================== + Coverage 81.97% 82.61% +0.63% ========================================== Files 32 36 +4 Lines 4538 5280 +742 ========================================== + Hits 3720 4362 +642 - Misses 818 918 +100 ```

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

imrn99 commented 1 month ago

@cedricchevalier19 I've added back the regular method and renamed the new ones as you suggested. Documentation is also updated for the new methods.

imrn99 commented 1 month ago

@cedricchevalier19

  1. I can definitely define an internal function with common code to all of these methods
  2. I don't have benchmark, but I have a setup in #164. What I can do is reverse changes in the kernel implementation (i.e. keep original methods) & merge both PRs. After this, I can benchmark the algorithm with both methods, looking at exec time and number of allocator calls
cedricchevalier19 commented 1 month ago

For 2, only if it is not too much work. Is performance the only expected gain, or do you think it can have other positive outcome, like better ordering for the ids?

imrn99 commented 1 month ago

For 2, only if it is not too much work.

I don't think it would take too much time, at least checking the number of allocator calls wouldn't

Is performance the only expected gain, or do you think it can have other positive outcome, like better ordering for the ids?

In sequential, there wouldn't be any change in ID ordering, but in parallel it would guarantee that all new darts are packed together. Original methods added darts as needed while new methods require a slice, which can always be made of contiguous IDs.