Conversation
9bd075a to
3a110a4
Compare
|
|
@m-brettell what is the status of this PR? Can it be reviewed and merged? |
3a110a4 to
5ce1a1a
Compare
krasznaa
left a comment
There was a problem hiding this comment.
Making it possible to use GBTS in the throughput applications would indeed be super helpful. Please do go ahead with this development!
I would prefer to do it differently though. 🤔 Instead of modifying all the existing full chain algorithms, you should just introduce a new algorithm just for CUDA (traccc::cuda::full_chain_algorithm_gbts?). And then build a separate set of executables (traccc_throughput_st_cuda_gbts, traccc_throughput_mt_cuda_gbts) using that full chain algorithm.
Once every language has GBTS available, we could re-visit this. But for now I think this would work better. 🤔
What? 😕 I thought we agreed to reduce the number of executables, not increase it. Mark's approach is perfectly reasonable and a lot more maintainable than what you are suggesting. |
I want to reduce the number of individual application source files. The throughput applications are very well synchronized in their code already. The new executables will need this code: #include "../common/throughput_mt.hpp"
#include "full_chain_algorithm_gbts.hpp"
int main(int argc, char* argv[]) {
// Execute the throughput test.
return traccc::throughput_mt<traccc::cuda::full_chain_algorithm_gbts>(
"Multi-threaded CUDA (GBTS) GPU throughput tests", argc, argv);
}And: #include "../common/throughput_st.hpp"
#include "full_chain_algorithm_gbts.hpp"
int main(int argc, char* argv[]) {
// Execute the throughput test.
return traccc::throughput_st<traccc::cuda::full_chain_algorithm_gbts>(
"Single-threaded CUDA (GBTS) GPU throughput tests", argc, argv);
}Please stop arguing with absolutely everything that I would suggest. |
Yes, and to make that happen we'll need to make a duplicate of the
I remind you that you are the one arguing with someone else's suggestion here. 😉 |
fa3ee41 to
c803e17
Compare
| layerInfoFile >> type; | ||
| layerInfoFile >> info[0] >> info[1]; | ||
| layerInfoFile >> geo[0] >> geo[1]; | ||
| layerInfo.addLayer(type, info[0], info[1], geo[0], geo[1]); |
There was a problem hiding this comment.
This will add type of ascii '0' and ascii '1', which have the int value of 48 and 49. Which as far as I can see it not intended.
There was a problem hiding this comment.
Added cast to fix, thanks.
|
It would be really useful to have this merged into main. |
|
|
Hi, it appears that it's not possible to add a GBTS config without modifying the existing full_chain_algorithms. All the constructors must have the same signature for the push_back in common/throughput_mt.ipp. Is there a way around this other than having dummy configs (as I do now)? Thanks. alg push_back |
|
Mentioning #1288 to Mark |
Indeed I don't think this is possible, at least not easily. Let's not waste time making this PR more complex now and let's go ahead with it as is, last Friday we all agreed that this PR was good to go as it is. |



Adding the option to use GBTS in the full_chain_alg -> throughput_mt
Reads in the needed layer linking scheme configuration information from the file
Also adds placeholder configurations to other backends to keep the signature of full_chain_alg's constructor consistent between backends.