-
Notifications
You must be signed in to change notification settings - Fork 20
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
Add propagate and reweight actions #289
Conversation
c17723e
to
de6e815
Compare
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.
@nahueespinosa how do you imagine motion and sensor models will look like moving forward? Free functions for the simplest models? Stateful functors for the complex ones? I see pass-by-value semantics followed by move-construction. Are we going to divorce behavior and state, and have the latter be passed by reference explicitly?
@hidmic In general I wouldn't divorce behavior and state unless we need to. I think we should be able to use stateful functors. But here I had a couple of potential implementation issues and I admittedly went for the lazy route. The arguments of In the overloads that create closures, if we're going to take a That, plus the fact that I had to re-order the arguments to allow for the execution policy to go first, made me think I would need an huge amount of code to do this properly. And I couldn't think of a quick way of reducing duplication. Before trying to implement this the hard way, I decided to give it a try to what the standard library does. It requires that function objects you pass to its algorithms be copyable. C++11 §25.1/10:
That means that if we have a stateful (and expensive to copy) functor, we should use the following expression: input |= beluga::actions::reweight(std::cref(sensor_model)); I know that the standard decision is from a time where perfect forwarding didn't exist and places the burden on the user, so I'm open to try to do it the hard way if you think it's worth it. Maybe it won't be as difficult as I think it will be. |
I have an idea... Will come back later. |
I was drowning in a glass of water. Stateful functors are now supported. 😅 |
df9f265
to
965f285
Compare
92a5757
to
c0aac24
Compare
Signed-off-by: Nahuel Espinosa <[email protected]>
Signed-off-by: Nahuel Espinosa <[email protected]>
Co-authored-by: Michel Hidalgo <[email protected]> Signed-off-by: Nahuel Espinosa <[email protected]>
Signed-off-by: Nahuel Espinosa <[email protected]>
Signed-off-by: Nahuel Espinosa <[email protected]>
Signed-off-by: Nahuel Espinosa <[email protected]>
c0aac24
to
4b61825
Compare
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.
LGTM
Proposed changes
Related to #279.
Type of change
Checklist