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

Exit with an error if the first smoothed state is invalid #882

Merged
merged 4 commits into from
Feb 22, 2025

Conversation

beomki-yeo
Copy link
Contributor

@beomki-yeo beomki-yeo commented Feb 21, 2025

Depends on #881.

This will solve the issue of seg fault issue in the benchmark by exiting with an error if the first smoothed state is invalid

Update: This does not solve the seg fault issue actually

@@ -226,6 +239,16 @@ class kalman_fitter {
// return false;
}
auto& last = *fitter_state.m_fit_actor_state.m_it_rev;

const scalar theta = last.smoothed().theta();
if (theta <= 0.f || theta >= constant<traccc::scalar>::pi) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make it clear, this is where the fix was done.

Copy link
Member

@krasznaa krasznaa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I absolutely like the direction that all of this is going in.

I was wondering for a moment whether we could make use of <system_error> in all of this, similar to what we do in Gaudi's StatusCode, but on a quick look std::error_code looks pretty GPU hostile. So simple enums are probably the best way to go about this. 🤔

I believe eventually we'll have a number of algorithm specific enums. And we'll need to think a bit more about the proper way of reporting back about these as well. But overall, I think this is a good direction to start in like this.

Just the unimportant question/suggestion from me, as shown below...

@@ -0,0 +1,20 @@
/** TRACCC library, part of the ACTS project (R&D line)
*
* (c) 2022-2024 CERN for the benefit of the ACTS project
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be:

Suggested change
* (c) 2022-2024 CERN for the benefit of the ACTS project
* (c) 2025 CERN for the benefit of the ACTS project

😉


#include <cstdint>
namespace traccc {
enum class kalman_fitter_status : uint32_t {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not in love with the name, but don't have fundamentally better suggestions for the moment. 🤔

But I wonder... Should we rather use std::uint32_t? I think the C++ standard only guarantees the existence of the std:: prefixed version.

https://en.cppreference.com/w/cpp/header/cstdint

But we certainly do this pretty inconsistently in the code elsewhere as well. 🤔

@stephenswat
Copy link
Member

Mind you this contains a bunch of already merged changes, the PR needs to be rebased.

@krasznaa
Copy link
Member

Mind you this contains a bunch of already merged changes, the PR needs to be rebased.

That of course explains things. I'm just surprised that (as long as it can be cleanly rebased) GitHub doesn't do this automatically in showing the diffs. I think GitLab can do that. Though I'm not absolutely sure about that...

@stephenswat stephenswat enabled auto-merge (squash) February 22, 2025 15:03
@stephenswat stephenswat merged commit a839774 into acts-project:main Feb 22, 2025
27 of 29 checks passed
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.

3 participants