Add empty pipeline constructor #332
Conversation
cpp/kiss_icp/core/Registration.hpp
Outdated
| int max_num_threads_; | ||
| // This attribute requires static to be able to manipulate the max | ||
| // concurrency from TBB across multiple instances of this class | ||
| static tbb::global_control tbb_control_settings; |
There was a problem hiding this comment.
@nachovizzo do you mean something like this?
There was a problem hiding this comment.
Yes. But doesn't need to be static then! I don't like it 😂 🤣 but I guess if you guys need the default constructor this is the only way
There was a problem hiding this comment.
I don't think the default constructor is really needed from our side, I think I wanted mostly to make it right, although a bit uglier. I think we can vote and decide altogether because this is not critical. But let's make it correct more so that beautiful but hidden.
There was a problem hiding this comment.
I think I wanted mostly to make it right
Why having a default constructor is the right way to do it? In the kiss-case I believe we provide such a dynamic and flexible way of parametrizing the system that allows users to construct the pipeline at runtime , where one doesn't even know what's going to process at compile time. With this in mind, we could completely get rid of the config struct. But I do like having the default parameters there. Even when I won't change it, I prefer to have an explicit initialization of the pipeline with those parameters. Choose conscientiously.
I think not having a default constructor had a reason, a philosophical one, meaning: We can't know the max range of your sensor at compile time, so we "force" you to specify this dynamically. Makes sense to me, but I don't have a strong opinion
There was a problem hiding this comment.
Basically yes, would be a matter of taste. I kind of liked the idea of not allowing to construction the pipeline with default parameters.
Not a big deal though, but then we shall remove the hack off the TBB number of threads and make it a registration member, this uglyfy quite a bit the API while being more correct at the same time.
Bro I was not saying that default constructing it is the right thing to do, I was referring to what you said yesterday when you say the attribute solution is more right but uglier. The default constructability does not add anything to the project from any point of view and can be definitely avoided. But if we can make even a little detail more right I think it is our philosophy to do it. I hope that clarity my view.
There was a problem hiding this comment.
All good, then if this is the target of the PR we should rename the PR or close/reopen. Otherwise, It's extremely confusing for me that open this at different intervals.
I'm happy if there is quorum to remove the static hack, but let's vote
There was a problem hiding this comment.
We can't know the max range of your sensor at compile time, so we "force" you to specify this dynamically.
Well, this we don't really enforce because we have default configs in both the pipeline header and the ROS launch file. The main idea of this PR was more: If you provide a default config, you can also provide a default constructor.
If we agree not to allow a default constructor, we should be more consistent and also not provide a default config. Otherwise, people can anyway just initialize with the default config.
There was a problem hiding this comment.
You are right, we are not enforcing per se. I don't believe anyone realizes that we have some parameters to tune 🤔 which is a success I'd say.
The main difference for the C++ side is that it's more likely users will indeed instantiate an object of the pipeline class, in contrast to the Python case where we don't expect that use case and people will use it more as a black-box system. One can't bake a KissICP object in Python without providing a config
If I'm writing C++ code, I'd love to see this error at compile time, and at least be conscious of constructing the object with a default config, which I don't even want to look into maybe, but understand that there is a config.
In either case, it's a matter of taste and decision
There was a problem hiding this comment.
Okay, that's a fair point.
Regarding making the tbb control setting an attribute I don't have a strong opinion, so I am fine with either renaming this PR or closing it.
There was a problem hiding this comment.
It seems none of us strongly need/want/desire the tbb control as an attribute or the default constructor. Let's close this PR for now and reevaluate later if something changes. thanks, guys
This PR allows you to initialize the pipeline without a config at hand by using the default KISSConfig. In that case, you still want to access the internal config, so we also added an accessor to it.