-
Notifications
You must be signed in to change notification settings - Fork 122
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
Initial support for expression templates in array and array_ref class #628
Conversation
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.
clang-tidy made some suggestions
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #628 +/- ##
=======================================
Coverage 94.04% 94.05%
=======================================
Files 43 43
Lines 6230 6236 +6
=======================================
+ Hits 5859 5865 +6
Misses 371 371 |
7da6de8
to
fdf2549
Compare
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.
clang-tidy made some suggestions
This improves the timing for the weighted sum benchmark by a factor of 3 (from around 650ns to about 200ns), but this can be improved further. For the benchmark of the simple case, this doesn't add any improvement mainly because it consists of a loop of the following form: for (int i = 0; i < dim; i++)
r += p[i]; The reason is that this current implementation evaluates I will send a separate PR trying this improvement. |
fdf2549
to
51b659f
Compare
clang-tidy review says "All clean, LGTM! 👍" |
include/clad/Differentiator/Array.h
Outdated
operator=(const array_expression<L, BinaryOp, R>& arr_exp) { | ||
assert(arr_exp.size() == m_size); | ||
for (std::size_t i = 0; i < m_size; i++) | ||
m_arr[i] = static_cast<T>(arr_exp[i]); |
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.
Why do we need the static_cast
here. Doesn't that signal a potential issue due to type incompatibility?
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.
removed 👍🏼
51b659f
to
f0f55a5
Compare
clang-tidy review says "All clean, LGTM! 👍" |
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.
This mostly looks good to me. Are we covered with tests? Perhaps we should write a benchmark making sure we do not regress the implementation?
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!
This is to speed up the operations for vector mode by ensuring that temporary array objects are not created when performing arithmetic operations of vectors.
Reference used: https://indico.cern.ch/event/450743/contributions/1116430/attachments/1224060/1790963/out-iCSC2016-L1.pdf