Track parameter estimation for cpu and cuda #85

beomki-yeo · 2021-09-07T05:01:57Z

traccc_pr_75 should be merged first before this PR

This is for track parameter estimation from found seeds.
The algorithm is almost copy-and-paste of EstimateTrackParamsFromSeed in Acts - a different thing is that the local position on surface is obtained from measurement directly while the acts one obtains it with global-to-local transformation and surface object.

There are a couple of features and issues that people might be interested

The size of eigen-based class/structure is measurement differently by cpu and cuda compiler which makes data transfer unstable and wrong. So the data size is synchronized by adding a data align qualifier (see core/include/definitions/qualifiers.hpp)
I initially put std::optional< measurement > member in spacepoint to refer it when I obtain the local track parameter, but it turned out that std::optional doesn't work in cuda device code - or anyone knows how to make it work??
The algebras used in track parameter estimation utilize algebra-plugin. But it seems it doesnt work with array plugin since the functions in algebra-plugin do not have host device qualifiers

Meanwhile I have observed that seed finding doesn't work with eigen_plugin and double scalar type ... I will look into that and make another PR to resolve it

TO-DO list:

measure the time for cpu and cuda
optimization

Update on Sep 22

This can be merged after #92

krasznaa

I already made a lot of comments, I would stop here for now. I think we'd need to sort out what we want to do with the container classes first. As right now I just don't think that they add any value over using the underlying vecmem types directly.

core/include/edm/collection.hpp

core/include/seeding/track_params_estimation_helper.hpp

krasznaa · 2021-09-07T08:28:12Z

device/cuda/include/cuda/seeding/seed_finding.hpp

@@ -55,7 +55,7 @@ struct seed_finding {
    /// Callable operator for the cuda seed finding
    ///
    /// @return seed_container is the vecmem seed container
-    host_seed_container operator()(
+    host_seed_container& operator()(


Wait... what? Are you returning a permanent object all of a sudden? This does not seem thread-safe at all... 🤔

The reason for this change was to use the return output of certain algorithm as an input of next algorithm directly. With copy method, the memory resource is missing so we cannot use it directly for the next algorithm.

auto output = cuda_algorithm(input) // output cannot be used as an input for the next algorithm bec

But as you mentioned that it is not thread-safe, I reverted it to copy method and do some modification:

algorithm::output_type output(mr); output = cuda_algorithm(input)

Is it okay to you?

Sure. (Sorry for the late answer... 😦)

krasznaa · 2021-09-07T08:30:22Z

device/cuda/include/cuda/seeding/track_params_estimation.hpp

+    track_params_estimation(vecmem::memory_resource* mr)
+        : m_mr(mr), params(mr) {}
+
+    output_type& operator()(input_type& i) {


Again, returning a non-const reference to a member object seems terribly thread-unsafe. I would've just opted for copy elision on return in this function...

answered above

krasznaa · 2021-09-07T08:37:21Z

examples/algorithms/cuda/include/cuda/track_finding/seeding_algorithm.hpp

@@ -22,7 +22,9 @@ class seeding_algorithm {
    using output_type =
        std::pair<host_internal_spacepoint_container, host_seed_container>;

-    seeding_algorithm(vecmem::memory_resource* mr = nullptr) : m_mr(mr) {
+    seeding_algorithm(vecmem::memory_resource* mr)


I guess this is not the only place where this pattern shows up, but this is where I'll comment about it...

We should only pass the memory resources around by pointer if it is allowed to pass a null pointer. In this case it's not really allowed anymore. So it would make for a better interface (in my opinion) to receive the memory resource as a reference. (vecmem::memory_resource&)

It is indeed annoying that the std classes receive memory resources through pointers. Since they do allow the pointers to be invalid. (In which case they fall back on a default resource.) But with vecmem in most cases we just need to pass explicitly created memory resources around. (Otherwise compilation on macOS can fail. As the Apple Clang compiler doesn't handle "default memory resources" quite correctly. Which provides a pretty good debugging tool for vecmem in the end. 😛)

Hey wait. If the input is reference, it's not polymorphic anymore for different memory_resource.

Do you just want it to be (vecmem::cuda::memory_resource& mr) for cuda algorithm and (vecmem::sycl::memory_resource& mr) for sycl algorithm?

References in C++ are the same in terms of polymorpism to pointers, so this should not be a problem. 🙂

OK thanks. The changes are applied

krasznaa · 2021-09-07T08:38:19Z

examples/algorithms/cuda/include/cuda/track_finding/track_params_estimation_algorithm.hpp

+
+    private:
+    output_type params;
+    std::shared_ptr<traccc::cuda::track_params_estimation> tp;


Is it really necessary to use a shared pointer here? Remember, these things are not cheap.

I don't think this is necessarily a problem here; this is a pointer to a function object, so it is unlikely to be replaced very often. Since std::shared_ptr has zero de-referencing overhead, I don't foresee any performance degradation here.

Whether it is really necessary is debatable, but I can see some benefit to this approach given that function objects in C++ are often assumed to be copyable. Using shared pointers here can feasibly reduce the overhead of that copying. But perhaps Beomki had different reasons in mind?

@stephenswat is right - This algorithm objects are generated only once before the actual algorithm runs. It could also be normal objects but I was too lazy to make a copy operator for algorithm classes. And if it doesn't introduce any overheads, we can just keep this way.

Just as for traccc::track_params_estimation_algorithm, I don't see what's the purpose of this class. 😕 What interface does it add on top of traccc::cuda::track_params_estimation?

Also, why does it not inherit from the traccc::algorithm base class? 😕

beomki-yeo · 2021-09-08T02:00:25Z

I update the algebra_plugins to the latest commit: the rotate function of eigen algebra is missing in the current version

beomki-yeo · 2021-09-08T04:10:23Z

Current PR compiles, but there is a runtime error in cuda track parameter estimation.
It must work when algebra_plugin_PR17 is merged, which adds inline to eigen functions

beomki-yeo · 2021-09-08T04:36:14Z

Naive estimation shows that the averaged track parameter estimation time is:
cpu: ~ 0.0009 sec
cuda: ~ 0.0012 sec

So cuda is slightly slower than cpu unfortunately. It's not a serious problem since the cuda seed finding time is usually 0.05 sec.
But let me profile and optimize it a bit for better performance.

stephenswat · 2021-09-10T09:54:45Z

If we're talking about 1.2 milliseconds here we're basically teetering on the border of what is achievable with memory transfer overhead and launch delays. Just following Amdahl's law I wouldn't necessarily recommend spending too much time optimizing this part of the code. Great job! 😄

beomki-yeo · 2021-09-10T17:46:49Z

@stephenswat OK. I will checkout whether the results are the same with Acts track parameter estimation before this PR goes in. Will let you know once it is done.

beomki-yeo · 2021-09-13T04:44:17Z

I added a test file tests/cpu/algorithms/compare_with_acts_seeding.cpp, which checks the results between acts and traccc seeding algorithm. Since I needed a hit file data so opened a traccc-data_MR1 in traccc-data repository.

@asalzburger or anyone who has write access to traccc-data can review the MR?

beomki-yeo · 2021-09-13T04:47:02Z

Other than than traccc-data_MR1, I also need algebra-plugin_PR17 to be merged to make this PR work (@niermann999 or anybody could have a look?)

beomki-yeo · 2021-09-13T20:10:13Z

Oh I made the MR in wrong repository. I moved it to the official one: traccc_data_mr_1

beomki-yeo · 2021-09-25T02:29:57Z

Are there more comments on this?

krasznaa

I'm still very much in favour of this development, but I think the code could be simplified a bit in a number of places...

krasznaa · 2021-09-28T08:35:02Z

device/cuda/include/cuda/seeding/seed_finding.hpp

    const seedfinder_config m_seedfinder_config;
    const seedfilter_config m_seedfilter_config;
    multiplet_estimator m_estimator;
    seed_filtering m_seed_filtering;

+    vecmem::memory_resource& m_mr;


I know, I know... I keep harping on about this one issue... 😦

In my mind a class that uses vecmem::memory_resource should look like:

class my_class { public: my_class( ..., vecmem::memory_resource& mr, ... ) : ..., m_mr( &mr ), ... my_class( const my_class& ) = default; my_class( my_class&& ) = default; my_class& operator=( const my_class& ) = default; my_class& operator=( my_class&& ) = default; vecmem::memory_resource& memory_resource() { return *m_mr; } const vecmem::memory_resource& memory_resource() const { return *m_mr; } private: vecmem::memory_resource* m_mr; };

So while its public interface should receive and hand out memory resources by reference (showing that the memory resource is an existing one for sure), internally it should use a pointer. This is because a reference member makes objects of the class non-copyable. Since references cannot be modified after creation. But if the member is a simple pointer, the default copy/move constructors/operators should work just fine for the class.

As I had to realise through acts-project/vecmem#114, it's probably an even better idea to use std::reference_wrapper<vecmem::memory_resource> as the variable that "points at" the memory resource. As that both ensures that we would initialise it correctly under all circumstances with a reference, and also allows us to copy/assign our own objects with such variables.

https://en.cppreference.com/w/cpp/utility/functional/reference_wrapper

So in all the places where I was complaining about vecmem::memory_resource member variables, the most ideal would seem to switch to using std::reference_wrapper<vecmem::memory_resource>.

Okay. definitely reference_wrapper would be better. Fixed now

krasznaa · 2021-09-28T08:41:47Z

device/cuda/include/cuda/seeding/track_params_estimating.cuh

@@ -0,0 +1,26 @@
+/** TRACCC library, part of the ACTS project (R&D line)


This is absolutely a question of convention... But I would rather call this file track_params_estimating.hpp. I think we should only use the .cuh extension on files that have actual CUDA code in them. Like a template class that launches kernels. (Which is not a good design idea, I just brought it up as an example.)

I myself took to organising my own code like the following recently:

helper_function.hpp: Declaration of the helper function, using regular C++;

helper_function.cu: Implementation of the helper function, using the CUDA runtime API, and implementing device code.

In such a setup one can come up with the relatively simple convention, that .cuh header files must only be included by .cu source files. As .cpp source files would not be able to interpret them. While .hpp header files can be included by both .cpp and .cu files.

I see if that's a usual convention. I modified all .cuh into .hpp

krasznaa · 2021-09-28T08:49:27Z

device/cuda/include/cuda/seeding/track_params_estimation.hpp

+namespace cuda {
+
+/// track parameter estimation for cuda
+class track_params_estimation {


🤔 I'm not sure what would be the best names, but traccc::cuda::track_params_estimating and traccc::cuda::track_params_estimation are way too similar.

But I'm also not sure what role this class is playing in general. 😕 Was this necessary for the previous implementation of how algorithms worked? In the current setup I think this is an unnecessary middle layer between trackccc::track_params_estimation_algorithm and traccc::cuda::track_params_estimating.

Right. I removed track_params_estimating since it is confusing as you mentioned. Instead, I directly implement operator() in track_params_estimation.cu. Now it will look much better.

For the algorithm part, I will comment out in the below as well; I removed the track_params_estimation_algorithm as it seems not needed for the moment. The initial motivation of algorithm one is for setting the configuration parameter rather than touching the original track_parameter_estimation in core directory. but It is apparent that we don't need the algorithm version because there is not any configuration parameter for track parameter estimation for the moment.

krasznaa · 2021-09-28T08:49:52Z

device/cuda/include/cuda/seeding/track_params_estimating.cuh

+/// @param resource vecmem memory resource
+void track_params_estimating(host_seed_container& seeds,
+                             host_bound_track_parameters_collection& params,
+                             vecmem::memory_resource* resource);


This should rather be a vecmem::memory_resource& reference.

Right. Anyway this file is removed

krasznaa · 2021-09-28T08:59:35Z

examples/algorithms/cpu/include/track_finding/track_params_estimation_algorithm.hpp

+
+namespace traccc {
+
+class track_params_estimation_algorithm


I'm continuing to be confused...

Is this class supposed to (eventually) decide between using the host or the CUDA implementation? If not, why does it exist? What additional value does it give on top of using traccc::track_params_estimation directly?

I'm probably missing something, but the naive UI in my head would be to have:

traccc::track_params_estimation_algorithm: An "algorithm" for running the track parameter estimation on the host.

traccc::cuda::track_params_estimation_algorithm: An "algorithm" for running the track parameter estimation using CUDA.

I think some of the layers introduced in this PR are not actually necessary. 🤔

As I explained above, these are removed now

krasznaa · 2021-09-28T09:02:39Z

examples/algorithms/cuda/include/cuda/track_finding/track_params_estimation_algorithm.hpp

+
+    private:
+    output_type params;
+    std::shared_ptr<traccc::cuda::track_params_estimation> tp;


Just as for traccc::track_params_estimation_algorithm, I don't see what's the purpose of this class. 😕 What interface does it add on top of traccc::cuda::track_params_estimation?

Also, why does it not inherit from the traccc::algorithm base class? 😕

krasznaa · 2021-09-28T09:09:52Z

examples/run/cpu/seq_example.cpp

+        traccc::clusterization_algorithm::input_type ca_input(cells_per_event);
+        traccc::clusterization_algorithm::output_type ca_output;
+
+        ca_output = ca(ca_input);
+
+        auto& measurements_per_event = ca_output.first;
+        auto& spacepoints_per_event = ca_output.second;
+
+        /*-----------------------
+             Seeding algorithm
+          -----------------------*/
+
+        traccc::seeding_algorithm::input_type sa_input(spacepoints_per_event);
+        traccc::seeding_algorithm::output_type sa_output;
+
+        sa_output = sa(sa_input);
+
+        auto& internal_sp_per_event = sa_output.first;
+        auto& seeds = sa_output.second;
+
+        /*----------------------------
+          Track params estimation
+          ----------------------------*/
+
+        traccc::track_params_estimation_algorithm::input_type tp_input(seeds);
+        traccc::track_params_estimation_algorithm::output_type tp_output;
+
+        tp_output = tp(tp_input);
+
+        auto& params = tp_output;


With the updated algorithms semantics this could surely be simplified quite a bit. Most of these lines should not be needed anymore.

krasznaa · 2021-09-28T09:12:35Z

tests/cpu/algorithms/compare_with_acts_seeding.cpp

+// Google Test can be run manually from the main() function
+// or, it can be linked to the gtest_main library for an already
+// set-up main() function primed to accept Google Test test cases.
+int main(int argc, char** argv) {
+    ::testing::InitGoogleTest(&argc, argv);
+
+    return RUN_ALL_TESTS();
+}


The test should rather be linked against GTest::gtest_main than to implement the main() function for ourselves.

I simply removed int main(..) since it was already linked to gtest. Not sure if it is what you meant though

beomki-yeo · 2021-09-29T00:14:18Z

I applied the update algorithm semantics to the seed finding as well as track parameter estimation even though it was not the initial purpose of this PR. I used the ravlue reference for the cuda algorithms since const lvalue reference doesn't work for vecmem containers. Maybe @stephenswat can look into the code to check if I didn't follow well the intention of the semantics.

I didn't edit spacepoint_grouping for the updated algorithm semantics but we can leave it as it is. Because I will make another PR to remove it and rewrite the algorithm with detray::grid2 once acts-project/detray#96 is merged

beomki-yeo · 2021-10-25T18:38:28Z

Squashed into one commit

beomki-yeo · 2021-11-12T00:39:44Z

Rebased with main branch.
With #103, the seq_example now runs for both eigen and array plugin.
However, This PR does not generate the execution files for different plugins with a single compilation - need to change cmake options to get execution file for different plugin. (Default option prioritizes the eigen plugin)

Event though it is not very desirable, let's make this happen in future PR... :)

pinging @krasznaa @stephenswat

p.s. overall run time does not change a lot between array and eigen plugin

stephenswat

Lots of good work here overall! It feels like there is a lot of unrelated work in this PR though - is that here for a reason or did it get included by accident? I also have some other comments, but otherwise I think this should be close to ready. 😄

stephenswat · 2021-11-12T00:42:57Z

core/include/clusterization/measurement_creation.hpp

-                m.variance = var / totalWeight;
+                m.variance = 1 / totalWeight * var;


Is there a reason to make this change? It seems to me like it would be slower and less numerically accurate. 😦

It is just becuase operator / is not defined for array in algebra-plugins

Will mark with TODO for later change

stephenswat · 2021-11-12T00:52:08Z

device/cuda/include/cuda/seeding/seed_finding.hpp

-#include <cuda/seeding/doublet_counting.cuh>
-#include <cuda/seeding/doublet_finding.cuh>
-#include <cuda/seeding/seed_selecting.cuh>
-#include <cuda/seeding/triplet_counting.cuh>
-#include <cuda/seeding/triplet_finding.cuh>
-#include <cuda/seeding/weight_updating.cuh>
+#include <cuda/seeding/doublet_counting.hpp>
+#include <cuda/seeding/doublet_finding.hpp>
+#include <cuda/seeding/seed_selecting.hpp>
+#include <cuda/seeding/triplet_counting.hpp>
+#include <cuda/seeding/triplet_finding.hpp>
+#include <cuda/seeding/weight_updating.hpp>


Why are we moving away from the .cuh extension? That's quite standard for CUDA programs. 😕

I thought so. But they don't have any cuda related code inside .cuh so decided to use .hpp extension for them.

This was one of my recommendations to use .hpp on headers that can be included by not only .cu files, but other C++ source files as well.

Since as I explained before, I like to use .cuh as a "contagious" extension. Marking something as .cuh should be done only when that particular header has code for launching a kernel, or when it includes some other .cuh file. In all other cases we should just use .hpp.

stephenswat · 2021-11-12T00:53:33Z

device/cuda/include/cuda/seeding/seed_finding.hpp

+    mutable std::shared_ptr<std::mutex> mutex{std::make_shared<std::mutex>()};
+    mutable host_doublet_counter_container doublet_counter_container;
+    mutable host_doublet_container mid_bot_container;
+    mutable host_doublet_container mid_top_container;
+    mutable host_triplet_counter_container triplet_counter_container;
+    mutable host_triplet_container triplet_container;
+    mutable host_seed_container seed_container;


I'm a little surprised to find this here. Are we relying on the internal state of our algorithms? I really think we should aim to have our algorithms be stateless!

I know.
Without these, I have to create host_doublet/triplet_container in operator() function for every event which makes entire algorithm much slower (it's almost 50% slowdown). Do you have a better idea on this?

device/cuda/include/cuda/seeding/seed_finding.hpp

stephenswat · 2021-11-12T00:54:52Z

device/cuda/include/cuda/seeding/seed_finding.hpp

+    /// @return seed_collection is the vector of seeds per event
+    output_type operator()(
+        host_internal_spacepoint_container&& i) const override {
+        std::lock_guard<std::mutex> lock(*mutex.get());


If we really need to do locking (which I hope we don't!) I think this is probably a little coarse-grained. It will be hard to get any decent acceleration from parallelism this way.

I also didn't want to make this change. The only way to remove mutable internal members whould be taking them as external parameters - but this is also coarse-grained.

And I don't think we have to achieve more parallelization in cuda operator(), which is grid bin level.

stephenswat · 2021-11-12T01:00:00Z

core/include/seeding/track_params_estimation_helper.hpp

+    auto uvTransform = [](const vector3& local) -> vector2 {
+        vector2 uv;
+        scalar denominator = local[0] * local[0] + local[1] * local[1];
+        uv[0] = local[0] / denominator;
+        uv[1] = local[1] / denominator;
+        return uv;
+    };


This function doesn't actually act as a closure - maybe we can just define it as a free function? Or maybe move it the algebra plugins?

Hmm I don't recommend moving it into algebra plugin.
I will just make a free function in the same file

device/cuda/src/seeding/track_params_estimation.cu

stephenswat · 2021-11-12T01:06:46Z

examples/algorithms/cuda/include/cuda/track_finding/seeding_algorithm.hpp

-class seeding_algorithm {
-    public:
-    using input_type = host_spacepoint_container&;
-    using output_type =
-        std::pair<host_internal_spacepoint_container, host_seed_container>;
+class seeding_algorithm
+    : public algorithm<
+          std::pair<host_internal_spacepoint_container, host_seed_container>(
+              host_spacepoint_container&&)> {


Are these changes related to the track parameter seeding in some way or have they snuck into this PR?

They snuck.

examples/run/cuda/seq_example_cuda.cpp

beomki-yeo · 2021-10-05T14:36:47Z

device/cuda/include/cuda/seeding/seed_finding.hpp

+    std::reference_wrapper<vecmem::memory_resource> m_mr;
+
+    // mutable internal objects for multiplets
+    mutable std::shared_ptr<std::mutex> mutex{std::make_shared<std::mutex>()};


People are okay with these mutable objects and mutex lock?
Without these, I have to declare host_doublet/multiplet_container in operator() function for every event which makes entire algorithm much slower (it's almost 50% slowdown)

beomki-yeo · 2021-11-12T01:40:56Z

core/include/clusterization/measurement_creation.hpp

-                m.variance = var / totalWeight;
+                m.variance = 1 / totalWeight * var;


It is just becuase operator / is not defined for array in algebra-plugins

beomki-yeo · 2021-11-12T01:44:41Z

device/cuda/include/cuda/seeding/seed_finding.hpp

-#include <cuda/seeding/doublet_counting.cuh>
-#include <cuda/seeding/doublet_finding.cuh>
-#include <cuda/seeding/seed_selecting.cuh>
-#include <cuda/seeding/triplet_counting.cuh>
-#include <cuda/seeding/triplet_finding.cuh>
-#include <cuda/seeding/weight_updating.cuh>
+#include <cuda/seeding/doublet_counting.hpp>
+#include <cuda/seeding/doublet_finding.hpp>
+#include <cuda/seeding/seed_selecting.hpp>
+#include <cuda/seeding/triplet_counting.hpp>
+#include <cuda/seeding/triplet_finding.hpp>
+#include <cuda/seeding/weight_updating.hpp>


I thought so. But they don't have any cuda related code inside .cuh so decided to use .hpp extension for them.

beomki-yeo · 2021-11-12T01:48:06Z

device/cuda/include/cuda/seeding/seed_finding.hpp

+    /// @return seed_collection is the vector of seeds per event
+    output_type operator()(
+        host_internal_spacepoint_container&& i) const override {
+        std::lock_guard<std::mutex> lock(*mutex.get());


I also didn't want to make this change. The only way to remove mutable internal members whould be taking them as external parameters - but this is also coarse-grained.

beomki-yeo · 2021-11-12T01:50:08Z

device/cuda/include/cuda/seeding/seed_finding.hpp

+    /// @return seed_collection is the vector of seeds per event
+    output_type operator()(
+        host_internal_spacepoint_container&& i) const override {
+        std::lock_guard<std::mutex> lock(*mutex.get());


And I don't think we have to achieve more parallelization in cuda operator(), which is grid bin level.

beomki-yeo · 2021-11-12T01:52:29Z

device/cuda/include/cuda/seeding/seed_finding.hpp

+    mutable std::shared_ptr<std::mutex> mutex{std::make_shared<std::mutex>()};
+    mutable host_doublet_counter_container doublet_counter_container;
+    mutable host_doublet_container mid_bot_container;
+    mutable host_doublet_container mid_top_container;
+    mutable host_triplet_counter_container triplet_counter_container;
+    mutable host_triplet_container triplet_container;
+    mutable host_seed_container seed_container;


I know.
Without these, I have to create host_doublet/triplet_container in operator() function for every event which makes entire algorithm much slower (it's almost 50% slowdown). Do you have a better idea on this?

beomki-yeo · 2021-11-12T01:53:45Z

device/cuda/include/cuda/seeding/track_params_estimation.hpp

+    output_type operator()(host_seed_container&& seeds) const override;
+
+    private:
+    std::reference_wrapper<vecmem::memory_resource> m_mr;


It's something I don't understand well. what is normal wrapper?

device/cuda/src/seeding/track_params_estimation.cu

beomki-yeo · 2021-11-12T01:58:49Z

examples/algorithms/cuda/include/cuda/track_finding/seeding_algorithm.hpp

-class seeding_algorithm {
-    public:
-    using input_type = host_spacepoint_container&;
-    using output_type =
-        std::pair<host_internal_spacepoint_container, host_seed_container>;
+class seeding_algorithm
+    : public algorithm<
+          std::pair<host_internal_spacepoint_container, host_seed_container>(
+              host_spacepoint_container&&)> {


They snuck.

beomki-yeo · 2021-11-12T02:48:59Z

is that here for a reason or did it get included by accident?

No good reason for that... :( I will try to make PR more clear in the future

beomki-yeo · 2021-11-30T10:29:46Z

Are there more comments on this? Otherwise I want it to get merged in

beomki-yeo · 2021-12-06T14:05:40Z

@stephenswat @krasznaa Are there more issues that I need to address?

stephenswat

Looks fine to me - just two small things I would like to follow up on. When that is done we can check whether Attila or anyone else has any comments, and then I would be in favour of moving to merge. 😄

core/include/definitions/primitives.hpp

stephenswat · 2021-12-06T15:19:47Z

core/include/seeding/track_params_estimation.hpp

+    }
+
+    private:
+    std::reference_wrapper<vecmem::memory_resource> m_mr;


Not a huge deal, but I still don't see the need to use reference wrappers here as opposed to normal wrappers.

That's something I am not sure - it is recommended by @krasznaa. I think he can explain us the motivation for this.

stephenswat

I'll sign off on this for now, but I will leave just a bit of time for others to comment if they want.

beomki-yeo · 2021-12-10T01:03:40Z

@stephenswat looks like there is no comment anymore. Can you get this in? We can come back to the wrapper thing at any time in the future

beomki-yeo · 2021-12-10T01:44:23Z

it's squashed into one commit

stephenswat

Let's do it! 👍

beomki-yeo changed the title ~~Track parameter estimation for cpu and cuda~~ WIP: Track parameter estimation for cpu and cuda Sep 7, 2021

krasznaa reviewed Sep 7, 2021

View reviewed changes

beomki-yeo changed the title ~~WIP: Track parameter estimation for cpu and cuda~~ Track parameter estimation for cpu and cuda Sep 7, 2021

krasznaa reviewed Sep 28, 2021

View reviewed changes

test time

ac4486f

beomki-yeo mentioned this pull request Oct 11, 2021

Refactor spacepoint binning #95

Merged

beomki-yeo force-pushed the track_params_estimation branch from bac67ba to 3f19089 Compare October 25, 2021 18:38

stephenswat added the feature New feature or request label Nov 11, 2021

stephenswat reviewed Nov 12, 2021

View reviewed changes

beomki-yeo commented Nov 12, 2021

View reviewed changes

beomki-yeo mentioned this pull request Nov 16, 2021

Arbitrary matrix definition in algebra plugin acts-project/algebra-plugins#40

Closed

stephenswat reviewed Dec 6, 2021

View reviewed changes

stephenswat approved these changes Dec 6, 2021

View reviewed changes

Merge branch 'main' of https://github.com/acts-project/traccc into main

6a0321c

add track parameter estimations

d414786

beomki-yeo force-pushed the track_params_estimation branch from 6c180c4 to d414786 Compare December 10, 2021 01:40

beomki-yeo mentioned this pull request Dec 10, 2021

Add link_type in container for indexing and its application to seeding #108

Merged

stephenswat approved these changes Dec 10, 2021

View reviewed changes

stephenswat merged commit af73a7b into acts-project:main Dec 10, 2021

		@@ -0,0 +1,26 @@
		/** TRACCC library, part of the ACTS project (R&D line)

		m.variance = var / totalWeight;
		m.variance = 1 / totalWeight * var;

Track parameter estimation for cpu and cuda #85

Track parameter estimation for cpu and cuda #85

Conversation

beomki-yeo commented Sep 7, 2021 • edited

Update on Sep 22

krasznaa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beomki-yeo commented Sep 8, 2021

beomki-yeo commented Sep 8, 2021

beomki-yeo commented Sep 8, 2021

stephenswat commented Sep 10, 2021

beomki-yeo commented Sep 10, 2021

beomki-yeo commented Sep 13, 2021

beomki-yeo commented Sep 13, 2021 • edited

beomki-yeo commented Sep 13, 2021

beomki-yeo commented Sep 25, 2021

krasznaa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beomki-yeo Sep 28, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beomki-yeo Sep 29, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beomki-yeo commented Sep 29, 2021 • edited

beomki-yeo commented Oct 25, 2021

beomki-yeo commented Nov 12, 2021 • edited

stephenswat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beomki-yeo commented Sep 7, 2021 •

edited

beomki-yeo commented Sep 13, 2021 •

edited

beomki-yeo Sep 28, 2021 •

edited

beomki-yeo Sep 29, 2021 •

edited

beomki-yeo commented Sep 29, 2021 •

edited

beomki-yeo commented Nov 12, 2021 •

edited