KFPackBuffer access to container

I am implementing a processor and I don't get the functionality of the FPackBuffer.

Its API only allows querying if a KF was notified in a specific timestamp.

Consider a KF created by a slow processor, how do we think the other processors should append their corresponding constraints to it?

I think that normally, there will be less KFpacks in the KFPackBuffer than captures in/dealt by the processor, wouldn't be useful iterating over the new KFPacks?

Am I understanding the whole think wrong?

After talking with @jsola, I saw i missunderstood the functionality of selectPackBefore(const TimeStamp& _time_stamp, const Scalar& _time_tolerance).

Its functionality is "selecting the first available pack if it's older than _time_stamp of within the _time_tolerance if it's newer than _time_stamp". So, maybe, it should be renamed to something like selectFirstPackIfOlder(...) or something that fits in a single line.. XD

Btw, its implementation is very confusing, I paste it below. Why not directly check container_.begin() instead of searching for the closest container element?

KFPackPtr KFPackBuffer::selectPackBefore(const TimeStamp& _time_stamp, const Scalar& _time_tolerance)
{
    if (container_.empty())
        return nullptr;

    KFPackBuffer::Iterator post = container_.upper_bound(_time_stamp);

    bool prev_exists = (post != container_.begin());

    if (prev_exists)
        return container_.begin()->second;
    else
    {
        bool post_exists = (post != container_.end());
        bool post_ok     = post_exists && checkTimeTolerance(post->first, post->second->time_tolerance, _time_stamp, _time_tolerance);

        if (post_ok)
            return post->second;
    }
    return nullptr;
}

I think this code is a simplification of the code in ProcessorTracker, where the premisse of returning the begin() pack is not valid. Here, effectively, I think we could do:

KFPackPtr KFPackBuffer::selectPackBefore(const TimeStamp& _time_stamp, const Scalar& _time_tolerance)
{
    // There is no pack
    if (container_.empty())
        return nullptr;
    
    // There exists a pack older than time stamp
    if (container_.begin()->first < _time_stamp)
        return container_.begin()->second;

    // All packs are more recent than time_stamp
    // then return the closest to time_stamp only if it is within time tolerance
    else
    {
        KFPackBuffer::Iterator post = container_.begin();

        bool post_ok = checkTimeTolerance(post->first, post->second->time_tolerance, _time_stamp, _time_tolerance);

        if (post_ok)
            return post->second;
    }
    return nullptr;
}

that is, if there is a pack older than time stamp, return the oldest pack. Otherwise, check for time tolerance and return the pack only if it lies within the time tolerance.

Yes, my point was "checking" on .begin() and return it if it's the case, not direclty returning it. Actually, an even more simple implementation would be:

KFPackPtr KFPackBuffer::selectPackBefore(const TimeStamp& _time_stamp, const Scalar& _time_tolerance)
{
    // Check if begin is older or within the time_tolerance of _time_stamp
    if (container_.empty() && 
         (container_.begin()->first < _time_stamp || 
          checkTimeTolerance(container_.begin()->first, container_.begin()->second->time_tolerance, _time_stamp, _time_tolerance) ) )
        return container_.begin()->second;

    return nullptr;
}

The problem with a radically simple implementation is that regular mortals do not understand the code. I prefer my verbose version!!!

Ok, I don't consider it is so unundestandable... We simply check if begin() is older or within the _time_tolerance of _time_stamp.

But ok, I propose the final one. I keep insisting because I think your proposal is not so clear specifically in the third case. Copying begin() in a variable called post is very weird and missunderstanding.

KFPackPtr KFPackBuffer::selectPackBefore(const TimeStamp& _time_stamp, const Scalar& _time_tolerance)
{
    // There is no pack
    if (container_.empty()
         return nullptr;

    // Checking on begin() since packs are ordered in time
    // First pack is older than time stamp
    if (container_.begin()->first < _time_stamp)
         return container_.begin()->second;

    // First pack is within the time tolerance
    if (checkTimeTolerance(container_.begin()->first, container_.begin()->second->time_tolerance, _time_stamp, _time_tolerance) 
        return container_.begin()->second;

    return nullptr;
}

What's the status of this?

For the new implementation proposed I think I was expecting an answer. It is not critical since it is just making the function more readable (to the best of my memory).

There is still the open issue of the function name.. this function takes the first pack if it is older than the time stamp provided (with a tolerance). I think selectPackBefore(ts,tol) does not mean this.

What about getFirstPackBefore(ts,tol)?

I'm implementing a MR solving a bug described in #180 (closed) precisely in these functions. I'll change the implementation to the new agreed (more readable). Please, confirm that getFirstPackBefore(ts,tol) is a good new function name instead of getPackBefore(ts,tol) and I also will change that. @jsola

OK to proposed implementation.

OK to fcn name.

mentioned in merge request !239 (merged)

We can continue in the MR !239 (merged)

closed