-
Notifications
You must be signed in to change notification settings - Fork 88
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
FaceSkeleton Vroom 1 #759
FaceSkeleton Vroom 1 #759
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #759 +/- ##
==========================================
+ Coverage 92.98% 93.12% +0.14%
==========================================
Files 33 33
Lines 4950 4949 -1
==========================================
+ Hits 4603 4609 +6
+ Misses 347 340 -7 ☔ View full report in Codecov by Sentry. |
Can we add some additional tests to check if the construction works out fine? Especially for the 1D case. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…into FaceSkeletonVroom1
Trying to improve performance a bit more julia> @benchmark Ferrite. _faceskeleton(topology, grid)
BenchmarkTools.Trial: 39 samples with 1 evaluation.
Range (min … max): 116.631 ms … 164.498 ms ┊ GC (min … max): 0.00% … 28.98%
Time (median): 129.052 ms ┊ GC (median): 0.00%
Time (mean ± σ): 128.925 ms ± 6.938 ms ┊ GC (mean ± σ): 0.95% ± 4.64%
█▁
▄▁▃▁▁▁▁▁▁▁▁▁▁▁▁██▆▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▃ ▁
117 ms Histogram: frequency by time 164 ms <
Memory estimate: 46.23 MiB, allocs estimate: 2. Maybe we can also get rid of the extra allocation (and GC?) |
Can you resolve the conflict? Should be good to go then. 10x improvement is really nice. :) |
…into FaceSkeletonVroom1
Co-authored-by: Maximilian Köhler <maximilian.koehler@ruhr-uni-bochum.de>
Makes face skeleton construction faster by using topology information.
Benchmarks:
Current implementation:
grid
argument is kept in case it's needed in the future. Theif-else
thing can be avoided if we consistently define "faces" as vertices for 1D case.