Use LvArray::math functions

[untereiner]

closes #2926

[TotoGaz]

Don't you think you should be using LvArray::math::abs instead

[untereiner]

Why? Someone might change one day the define real32 ?

[untereiner]

What about sqrt and pow? I don’t see any using namespace Lvarray::math. That would be koenig’s lookup aware by the way

[TotoGaz]

Why? Someone might change one day the define real32 ?

Because the LvArray::math functions are supposed to be GPU proof. Which makes sense in a __device__ function.

[klevzoff]

std::abs will not compile on device. Sadly, LvArray::math::abs does not provide correct overload for floating point values either (it dispatches to ::abs(int) on device which causes truncation).

You have 2 options:

• use fabsf (no namespace)
• or implement correct overloads in LvArray/src/math.hpp (dispatch to ::fabsf for float and ::fabs for double) and use them.

sqrt, pow, fabs, etc. are functions in the global namespace inherited from the C standard. On device compilation trajectory, device compilers substitute their own versions, so these are always safe to use. The value of LvArray::math versions is in generic code, since they can usually dispatch appropriately depending on argument type (e.g. to fabsf vs fabs vs abs).

[untereiner]

@TotoGaz @klevzoff is last commit acceptable ? If a pow function is added to lvarray it will be used first.

[klevzoff]

It's all good by me as long as it compiles on all platforms :)

Note that adding using namespace LvArray::math does not necessarily mean the LvArray version will be called, it just adds another set of templates/overloads in the set and lets overload resolution pick the winner. For example, given

double pow(double base, double exp);

in the global namespace coming from C stdlib, and

template<typename T>
T pow(T base, T exp);

in LvArray::math, the first one will be picked (since a non-template perfect match wins over a template). The end result is the same though. The details about when C stdlib functions are added or not to the global namespace are notoriously murky and even less clear with CUDA (we seem to get more than is documented). The whole situation is a mess :) The one thing we should always avoid is using std:: version in kernel code, which this PR correctly does.

[corbett5]

I don't know if this PR (or anywhere else for that matter) has using namespace LvArray::math, but for exactly that reason it should be avoided. Using LvArray::math::pow is not a elegant solution but at least there is zero ambiguity.

[TotoGaz]

OK, so let's wait for @untereiner to explicitly use LvArray::math.

Maybe using LvArray::math::pow;, using LvArray::math::abs;... can help at having the formula a bit short?

In the long term, maybe could we have a geos::math namespace that would allow us to forward the LvArray::math namespace, so its usage will be more natural?

[untereiner]

Thank you @klevzoff. I thought the opposite: if you bring functions from a namespace into scope then they would be prioritized. Maybe the C versions should be put in a namespace and should not hang around and pollute the global one.

[CusiniM]

This does not seem to be ready to be merged. Neither @TotoGaz nor @rrsettgast have approved it. And it looks like @TotoGaz had some comments.

[TotoGaz]

This LGTM. I'm waiting for @klevzoff 's expertise though!

[untereiner]

as a side note, if this solution is acceptable it should be added to another places.

[codecov]

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (3ccaaf9) 51.33% compared to head (402286d) 51.42%.
Report is 1 commits behind head on develop.

❗ Current head 402286d differs from pull request most recent head 2daac51. Consider uploading reports for the commit 2daac51 to get more accurate results

Files	Patch %	Lines
...agation/ElasticFirstOrderWaveEquationSEMKernel.hpp	0.00%	3 Missing ⚠️
...s/wavePropagation/ElasticWaveEquationSEMKernel.hpp	0.00%	3 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #2927      +/-   ##
===========================================
+ Coverage    51.33%   51.42%   +0.08%     
===========================================
  Files          968      966       -2     
  Lines        86819    86672     -147     
===========================================
+ Hits         44570    44571       +1     
+ Misses       42249    42101     -148     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.