-
Notifications
You must be signed in to change notification settings - Fork 86
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
Add zero-checks to axpy-like operations #1573
base: develop
Are you sure you want to change the base?
Conversation
This prevents NaNs from polluting the output
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
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 in general. Maybe there are still some static_cast
missing to make it compile.
[&beta_val](const type& x) { | ||
return is_zero(beta_val) ? zero(beta_val) : beta_val * x; | ||
}); |
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.
minor performance comment, you could try switching the lambda and zero check, i.e.
is_zero(beta)
? [&beta_val](const type& x) { return zero(beta); }
: [&beta_val](const type& x) { return beta_val * x; }
But this might not work, since the two branches of the ?: operator have different types. And it might increase compile times, since it might compile the kernel two times
arithmetic_type result = c->at(row, j); | ||
result *= beta_val; | ||
arithmetic_type result = | ||
is_zero(beta_val) ? zero(beta_val) : beta_val * c->at(row, j); |
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.
is_zero(beta_val) ? zero(beta_val) : beta_val * c->at(row, j); | |
is_zero(beta_val) ? zero(beta_val) : beta_val * static_cast<arithmetic_type>(c->at(row, j)); |
to make it compile
Is there any reason to avoid propagation of NaN? If there's no performance penalty, I think propagation of NaN is easier to know the algorithm does not work out due to some arthmetic error. |
@yhmtsai if you are computing for example |
no, 0 * NaN should be NaN not zero, so it is not mathimatical equality by just leaving them out. |
I know current vendor library usually treat 0 as do not touch due to BLAS. |
that makes calculations more fragile, we already do similar things (special cases) for zeros inside our solver kernels |
@yhmtsai we treat |
Operations like$\alpha A x + 0 y$ may propagate NaNs from y to the output despite the 0 coefficient. This can be avoided by checking the beta scaling factors for zero explicitly.
TODO: