-
Notifications
You must be signed in to change notification settings - Fork 75
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
NEW Added tests for existing math operators #847
Conversation
thanks @jkrude. In our case we took a more involved code to keep the definition and extensions of math operations compact and to learn C++11. I am happy, that you use patterns of meta-programming, so you are getting into Alpaka :) You also document the code, that's good! However, the code is too complex and error-prone to leave it untested itself, e.g. how do I know, that the arguments' data are filled correctly? |
Can #828 be closed now? |
|
There are ome compilation errors when clang is used as native CUDA compiler:
|
Is anyone working on fixing the compilation issues? |
@jkrude PLease rebase this PR against the current dev and squash all commits. |
5937221
to
b5b6051
Compare
Some of the windows builds seem to have problems like:
after line 37 in |
I created a PR to fix the Windows bigobj issue: #979 |
Added tests for unary math operators with functors. Added binaryOps and reworked unaryOps. Reduced doubled code significantly, by allowing to test operators of n-arity with the same code. Integrated PR from td11235813 Removed HCC workaround and rebased NOTE: fast-math needs to be disabled for gpu-backend. Co-authored-by: Matthias Werner <Matthias.Werner1@tu-dresden.de>
#include "../include/Defines.hpp" | ||
#include "../include/Buffer.hpp" | ||
#include "../include/Functor.hpp" | ||
#include "../include/DataGen.hpp" |
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.
Sorry, I did not notice this in time. I believe it's better to avoid relative paths here, and also in other files added by this PR, and instead do #include <alpaka/test/unit/math/mathOps/include/Defines.hpp>
. What do you think @jkrude @BenjaminW3 @psychocoderHPC ?
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.
The CMake way would be to add the include folder to the target_include_directories
of the test target. Then you do not need relative includes anymore.
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.
But on the other hand those files are no public includes at all. We should simply move them into the src
folder.
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.
Moving them into src
is IMO ok. The current relative path requires that the install folder structure is equal to the structure we have in our source code. We should not assume it.
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.
@jkrude when you have time, could you implement this change? Should be not much work, just moving the files and fixing the includes.
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.
Rework of #828
Tests were run on a cuda-device and std-backend.
See also https://github.com/ComputationalRadiationPhysics/alpaka/pull/828 for the previous version.
Implemented functors and abstraction, so that one code is used for all operators.