-
Notifications
You must be signed in to change notification settings - Fork 1
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
refactor(core): improve grid building routines #209
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #209 +/- ##
==========================================
+ Coverage 82.04% 82.60% +0.56%
==========================================
Files 41 41
Lines 5681 5865 +184
==========================================
+ Hits 4661 4845 +184
Misses 1020 1020 ☔ View full report in Codecov by Sentry. |
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.
I was just wondering if splitting the loop into (the interior domain and then the boundary (last row & last column)) can not make the code easier to read (and perhaps more efficient without a conditional in the middle).
(0..n_square_y) | ||
.flat_map(|y_idx| (0..n_square_x).map(move |x_idx| (y_idx, x_idx))) |
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.
Comment on why flatten the iterations.
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.
done in 5d61aa9
|
||
// and then build faces | ||
assert_eq!(map.fetch_faces().identifiers.len(), n_square_x * n_square_y); | ||
debug_assert_eq!(map.fetch_faces().identifiers.len(), n_square_x * n_square_y); |
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.
Leave a comment stating why it can impact the performances.
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.
done in 5d61aa9
extract some function common to all cases
I managed to reorganize the code by splitting like you suggested. I also extracted some common functions which should help with readability It does not seem to make much of a difference performance-wise (maybe 5% better? but laptop numbers aren't really reliable) |
link
usages withset_beta
usagesperformance changes for a 512x512 grid (exec time in ms):
157.11
89.83
92.98
24.54
246.48
120.16
121.79
35.54