Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rcb: remove async implementation #165

Merged
merged 5 commits into from
Jun 7, 2022
Merged

rcb: remove async implementation #165

merged 5 commits into from
Jun 7, 2022

Conversation

hhirtz
Copy link
Member

@hhirtz hhirtz commented Jun 7, 2022

and improve the simple one by:

  • using geometry::BoundingBox to keep track of min and max
  • keep track of the sum of each split
  • use crate::partial_cmp to use half as much float comparisons
  • don't push work on rayon's back for small sets (currently, point sets with less than 4096 elements)

depends on #164

@codecov
Copy link

codecov bot commented Jun 7, 2022

Codecov Report

Merging #165 (048e29f) into master (28272a9) will decrease coverage by 0.73%.
The diff coverage is 97.85%.

@@            Coverage Diff             @@
##           master     #165      +/-   ##
==========================================
- Coverage   51.74%   51.01%   -0.74%     
==========================================
  Files          42       41       -1     
  Lines        7251     6979     -272     
==========================================
- Hits         3752     3560     -192     
+ Misses       3499     3419      -80     
Impacted Files Coverage Δ
ffi/src/lib.rs 0.29% <ø> (+<0.01%) ⬆️
src/lib.rs 100.00% <ø> (ø)
tools/src/lib.rs 0.26% <ø> (+<0.01%) ⬆️
src/algorithms/recursive_bisection.rs 92.48% <97.85%> (+8.33%) ⬆️
src/algorithms/vn/first.rs 72.11% <0.00%> (-1.20%) ⬇️
src/geometry.rs 96.21% <0.00%> (+0.23%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 28272a9...048e29f. Read the comment docs.

for simplicity's sake

Zoltan2 uses the same BSP paradigm + data movements, so the simple_rcb
version is state of the art.  A next step would be to improve data
locality with custom rayon drivers.

This commit removes the relevant dependencies, and also removes the
indirect dependency on proc-macros via nalgebra-macros and tracing.
instead, keep track of the bounding boxes and the sum of the parts.

This reduces the load of high-iter-count runs.
Since this makes half as many float comparisons and select_nth_unstable
ends up only matching for Ordering::Less anyway:
https://github.com/rust-lang/rust/blob/cb0584f86b8cfa952dffad55f7d83bd90765120f/library/core/src/slice/mod.rs#L2719
It's slower to go through rayon than to sort+split them.
@hhirtz
Copy link
Member Author

hhirtz commented Jun 7, 2022

Codecov seems to complain because lot of previously-covered lines were deleted? Though the coverage diffs are all on the positive side (except for the spurious vnfirst :/)

@hhirtz hhirtz merged commit bc0602f into master Jun 7, 2022
@cedricchevalier19 cedricchevalier19 deleted the rcb-tidy branch June 22, 2022 13:17
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.

1 participant