-
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
refactor!: Fitters use loggers more consistently #1727
refactor!: Fitters use loggers more consistently #1727
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1727 +/- ##
==========================================
- Coverage 49.28% 49.14% -0.14%
==========================================
Files 395 395
Lines 21810 21800 -10
Branches 9929 9953 +24
==========================================
- Hits 10748 10713 -35
- Misses 4179 4187 +8
- Partials 6883 6900 +17
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
📊 Physics performance monitoring for 141f18cFull report VertexingSeedingCKFAmbiguity resolutionTruth tracking (Kalman Filter)Truth tracking (GSF) |
I would propose overriding the coverage check here. |
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.
No significant comments.
Maybe one more philosophical. Should the logger be const in fact?
I.e. it is all about changing the state/side effects. I guess it has to in order to be usable in const methods.
You're right that it technically has side-effects. The original decision predates me ( 😉 ) but I think the |
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.
took a quick look - I think that makes a lot of sense
Co-authored-by: Andreas Stefl <stefl.andreas@gmail.com>
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.
lets get this in
First follow up to acts-project#1724 (includes that PR as well), which switches the KF, CKF, GSF and GX2F to use `Logger` directly, getting rid of the `LoggerWrapper`. This uses the new clone mechanism to create child loggers for the actor. To remove the logger instance from the propagation options, I had to change all the actors and aborters, to take the logger as an explicit argument. I changed the ActionList and AbortList to just forward any arguments that are passed into the call operator, as that should be more flexible. Since this makes the sort-of-concept-style call signature checks very complex, I tentatively removed those as well now. I think it's fine to drop these, they don't provide too much value here, and the action and abort list are in need of an actual reimplementation anyway.
First follow up to #1724 (includes that PR as well), which switches the KF, CKF, GSF and GX2F to use
Logger
directly, getting rid of theLoggerWrapper
. This uses the new clone mechanism to create child loggers for the actor.To remove the logger instance from the propagation options, I had to change all the actors and aborters, to take the logger as an explicit argument. I changed the ActionList and AbortList to just forward any arguments that are passed into the call operator, as that should be more flexible. Since this makes the sort-of-concept-style call signature checks very complex, I tentatively removed those as well now. I think it's fine to drop these, they don't provide too much value here, and the action and abort list are in need of an actual reimplementation anyway.