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

Protect against negative chi-square in updater #895

Merged
merged 1 commit into from
Feb 27, 2025

Conversation

stephenswat
Copy link
Member

I've observed an issue in which the gain matrix updater sometimes produces $\chi^2$ values less than zero, which makes no sense. This commit adds a check for this condition and returns an appropriate error state.

@stephenswat stephenswat added the bug Something isn't working label Feb 25, 2025
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.

Up to Beomki to say the final word, but from my side, sure, this looks sane.

I don't think it makes any difference for the GPU compilers, but would it make sense to add [[unlikely]] on these checks? 🤔

https://en.cppreference.com/w/cpp/language/attributes/likely

@beomki-yeo
Copy link
Contributor

beomki-yeo commented Feb 25, 2025

Rigorously speaking, it is an error. The question is how frequently this happens. If it happens for 99% of tracks.. maybe we should ignore 💀

@beomki-yeo
Copy link
Contributor

Meanwhile, the error also needs to be included in the two filters smoother :)

@stephenswat
Copy link
Member Author

I don't think it makes any difference for the GPU compilers, but would it make sense to add [[unlikely]] on these checks? 🤔

https://github.com/acts-project/traccc/pull/886/files 😉

@stephenswat
Copy link
Member Author

Meanwhile, the error also needs to be included in the two filters smoother :)

I've updated the two-filter smoother, now!

Copy link
Contributor

@beomki-yeo beomki-yeo left a comment

Choose a reason for hiding this comment

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

I like the idea while we can still make it more informative

I've observed an issue in which the gain matrix updater sometimes
produces $\chi^2$ values less than zero, which makes no sense. This
commit adds a check for this condition and returns an appropriate error
state.
@stephenswat
Copy link
Member Author

Okay, I've updated the return values.

@stephenswat stephenswat merged commit 625cb07 into acts-project:main Feb 27, 2025
23 of 29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants