Skip to content
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

Inflate Covariance in CKF #846

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

beomki-yeo
Copy link
Contributor

While working on #844, I found that I didn't add covariance inflation in the CKF. This PR adds it for CPU CUDA and SYCL implementations.

It seems thrust does not support the lambda expression by default so I defined a struct for covariance inflation to make a functor.
However, SYCL does not seem to work with a functor. For example, when I use the following code:

    covariance_inflator cov_inf(config.covariance_inflation_factor);
    oneapi::dpl::for_each(policy, in_params_device.begin(),
                          in_params_device.end(), cov_inf);

I got the following error:

/opt/intel/oneapi/dpl/2022.7/include/oneapi/dpl/pstl/hetero/../hetero/dpcpp/parallel_backend_sycl.h:242:17: error: no matching function for call to object of type 'const oneapi::dpl::unseq_backend::walk_n<oneapi::dpl::execution::device_policy<> &, traccc::covariance_inflator>'

If I use the lambda expression for SYCL like in this PR, the compilation takes forever... any ideas?

@beomki-yeo beomki-yeo changed the title Inflate covariance ckf Inflate Covariance in CKF Feb 8, 2025
@beomki-yeo beomki-yeo force-pushed the inflate-covariance-ckf branch from 795f8d4 to 8f0a126 Compare February 8, 2025 07:37
@beomki-yeo beomki-yeo force-pushed the inflate-covariance-ckf branch from 8ebfd75 to 0fd99c0 Compare February 8, 2025 07:47
@beomki-yeo beomki-yeo force-pushed the inflate-covariance-ckf branch from 0fd99c0 to b342339 Compare February 8, 2025 07:48
Copy link

sonarqubecloud bot commented Feb 8, 2025

@beomki-yeo beomki-yeo marked this pull request as draft February 8, 2025 11:13
@beomki-yeo
Copy link
Contributor Author

beomki-yeo commented Feb 9, 2025

The increased covariance makes CKF pick up the measurements very far from the track. ($$\chi^2 \propto distance^2/covariance$$). Eventually CKF will produce lots of fake tracks and increase the wall time significantly 🤔
I will leave this PR draft until I find a solution

@stephenswat
Copy link
Member

Would it help if we found measurements in a sorted way, i.e. we select the $N$ best measurement (ranked by $\chi^2$) instead of what we do now which is selecting $M$ random measurements within a $\chi^2$ cut; with $N &lt; M$?

@stephenswat
Copy link
Member

Note this is something I have discussed as being useful with @paradajzblond as well.

@beomki-yeo
Copy link
Contributor Author

We already have max_num_branches_per_surface defined in finding_config.hpp, so it won't be very difficult. Maybe we should have implemented that already?

However, I am afraid this approach still does not solve the issue from this PR. CKF will still make branches as many as $N_{max}$.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants