-
Notifications
You must be signed in to change notification settings - Fork 157
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
fix: Protection against wrong configuration of Seed finder config #2129
fix: Protection against wrong configuration of Seed finder config #2129
Conversation
📊 Physics performance monitoring for ea16fb6Summary VertexingSeedingCKFAmbiguity resolutionTruth tracking (Kalman Filter)Truth tracking (GSF) |
Codecov Report
@@ Coverage Diff @@
## main #2129 +/- ##
=======================================
Coverage 49.44% 49.44%
=======================================
Files 436 436
Lines 25125 25125
Branches 11607 11607
=======================================
Hits 12424 12424
Misses 4467 4467
Partials 8234 8234 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. You were wondering if we ever want to actually configure the finder without the filter, I guess we don't? Thoughts @LuisFelipeCoelho ?
I also don't see why we would want to configure the seed finder without the filter. In any case, we can configure the filter to keep all seeds. |
looks like our tests rely on this 🤔 |
ok, then I'll get a look at the failures here and fix them. It looks like indeed we have situation where the filter is not set |
8b3505f
to
37264ec
Compare
I forgot in the previous commit to propagate the changes in the SeedingAlgorithm example to this MR. Now it all works |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great, thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole "internal units" mechanism is a bit of a crux that we should be looking to remove. That's independent of this change though.
In case a misconfiguration of the
SeedFinderConfig
happens, and theseedFilter
is a nullptr, the code crashes.This PR adds a protection against this eventuality. This happens in the
toInternalUnits
function, that now takes responsibility for checking the validity of its parameters.It checks that the seedFilter is not a
nullptr
and that its config is already in internal units.