Add MCL core class #428
Add MCL core class #428
Conversation
Signed-off-by: Ramiro Serra <[email protected]>
| typename WeightT = beluga::Weight, | ||
| class ParticleType = std::tuple<typename SensorModel::state_type, WeightT>, | ||
| class ExecutionPolicy = std::execution::sequenced_policy> | ||
| class Mcl { |
There was a problem hiding this comment.
@serraramiro1 @nahueespinosa meta: could beluga::Amcl be based on beluga::Mcl?
There was a problem hiding this comment.
I remember having a quick talk about using a CRTP base class for this. It seemed like there wasn't much to reuse, but looking at the code, there might be enough to justify a base class?
There was a problem hiding this comment.
There is substantial overlap, though I'm glancing over the details of how we would reuse the code.
| class MotionModel, | ||
| class SensorModel, | ||
| typename WeightT = beluga::Weight, | ||
| class ParticleType = std::tuple<typename SensorModel::state_type, WeightT>, |
There was a problem hiding this comment.
@serraramiro1 @nahueespinosa meta: moving forward, this looks unnecessarily restrictive. SensorModel could work with multiple state types. Same can be said about MotionModel.
There was a problem hiding this comment.
This is not a constraint though, users can choose to set the parameter explicitly if they need to. Are you proposing that these classes do not store the particle set? That would remove the need for this.
There was a problem hiding this comment.
I'm wondering if we should couple the particle type and the models like this. Models could work with multiple types at the same time (e.g. through generic lambdas), so sparing the user from explicitly setting the particle state type on class instantiation seems more trouble than it's worth.
Proposed changes
Addresses #424
Type of change
Checklist
Put an
xin the boxes that apply. This is simply a reminder of what we will require before merging your code.