-
Notifications
You must be signed in to change notification settings - Fork 10
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
Simplify dependency structure #146
base: main
Are you sure you want to change the base?
Conversation
0878a32
to
3b3f425
Compare
This commit significantly simplifies the dependency structure by removing the backends for vecmem and smatrix, as well as frontends which combine non-matching frontends and backends. This is functionality we never ever use and it is simply complicating this repository.
3b3f425
to
6eba104
Compare
|
virtual void SetUp() override { | ||
|
||
// Create the vectors. | ||
CUDA_ERROR_CHECK(cudaMallocManaged( |
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 am not on board with this.
I agree with getting rid of the "vecmem frontend" of the code. But we still shall continue to use vecmem to test the code. Since we're using vecmem for the memory management of everything in all the downstream projects as well. So we should test the synergy between these codebases already here.
I.e. the project will definitely need to continue building vecmem for its tests.
Note that once we merge the project into detray, this is not "anything" even. Since detray is not going to stop depending on vecmem either.
#pragma once | ||
|
||
// Project include(s). | ||
#include "algebra/math/eigen.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.
You were a bit over-eager with this one as well I fear. 🤔
Let's discuss which frontends we want to get rid of. I don't want to throw out the baby as well in all of this...
add_library( algebra::tests_sycl_common ALIAS algebra_tests_sycl_common ) | ||
|
||
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fsycl") |
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.
Come on... No. This is not flying like this. Why did you think that hardcoding the use of just oneAPI like this is the right move? 😕
This commit significantly simplifies the dependency structure by removing the backends for vecmem and smatrix, as well as frontends which combine non-matching frontends and backends. This is functionality we never ever use and it is simply complicating this repository.