filters
filters copied to clipboard
ros
`set_capacity` not defined for `realtime_circular_buffer`?
Am I right or set_capacity() member is not implemented? I searched in the whole repo, only one result for this symbol, and it's the line at the link below.
https://github.com/ros/filters/blob/e33463aebca78471c0e52f45eaf31585b781af61/include/filters/realtime_circular_buffer.hpp#L83C8-L83C20
In this case, would you like me to provide a PR?
@jonbinney The seems to be a bit of ambiguity about the original meaning of this function.
In Boost:
- set_capacity acts as a
reserve()call in STL - resize acts as a
resize()call in STL
Given the signature of the missing set_capacity() implementation here, I believe that it should call resize() internally. Since we would not break any API here, we could think about renaming this method to avoid any confusion.
From the boost documentation of resize(): "If the new size is greater than the current size, copies of item will be inserted at the back of the of the circular_buffer in order to achieve the desired size."
That doesn't sound like the behavior I'd usually want. What use case do you have in mind? I wouldn't worry too much about keeping the signature of set_capacity() the same here.
@jonbinney IMHO new items should be somehow initialized, so the user has to provide a value.
My use case is that I implemented an average filter on top of this buffer, by averaging latest N samples of an incoming measure. I would not change size of the buffer at runtime, but it would come handy to change the size of a default-constructed one during for example initialization of a controller - like, on_init()/on_configure() stages.
That doesn't sound like the behavior I'd usually want.
Could you elaborate a bit? Let's discuss what is your ideal behaviour, maybe we can implement it inside this class here.
I may misunderstand, but wouldn't your rolling window average be wrong at the start? For example, assume N=10 for your case. You resize() the buffer to a size of 10 , fill it with some default value, and start collecting samples. When you have collected 5 samples so far, how would you know that the other 5 samples aren't yet actual samples? The size would be 10 and the capacity would be 10, so they're effectively storing the same information twice.
If we have a set_capacity() method, in the same case capacity would be 10 and size would be 5, so you'd have more info - you'd know exactly how many valid samples you've got, right?
The point is: let's say we construct a RealtimeCircularBuffer(5,0). If we only do set_capacity(10) on the internal buffer later on, the user will not be able to add a sixth element at all.
About the averaging in my use case: in the first few cycles I may be able to accept a transient with "wrong values" (a sort of ramp effect), but truly I use a counter to keep track of the number of real measures obtained up to that moment (i.e. something that grows like 1 2 3 4 5 5 5 5 5 5...)
You're right - I misunderstood how push_back() worked! Also the constructor of RealtimeCircular buffer already requires initializing the elements, so having the resize method do the same makes sense. I agree now that having a resize() method makes more sense.
There is still a thing to notice:
- since we expose both
push_front()andpush_back()the user is free to use the circular buffer in both directions - i.e. when pushing back an element, the buffer might drop the front one (if it's already full), when pushing front an element, it might drop the back one instead.
Given this, the problem here is to choose between resize and rresize (notice the double R). Because people might want to reside extending the left-most side, or the right-most one, depending on the direction they are using the buffer (front->back or back->front).
@jonbinney do you have any opinion or more background on this?
It looks like both the mean and median filters in this repo use push_back(); I'd guess that is the most common case.
I would propose to drop RealtimeCircularBuffer::push_front() and expose boost::circular_buffer::resize().
This way users are forced to use the circular buffer in one direction only.
But I can imagine breaking some possible packages in the wild doing so...
I should have asked this earlier, but at some point is it worth encouraging people to just use boost::circular_buffer directly instead of exposing more and more of its member functions in RealtimCircularBuffer?