end_time excludes messages that occur at the same time

[airalcorn2]

Description

• Version: 1.2.1
• Platform: ROS 2 Iron Irwini

Steps To Reproduce

from mcap.reader import make_reader
from mcap_ros2.decoder import DecoderFactory

mcap_path = "some_mcap.mcap"
some_topic = "some_topic"
first_ns = None
with open(mcap_path, "rb") as mcap_f:
    reader = make_reader(mcap_f, decoder_factories=[DecoderFactory()])
    for _, _, ros_msg in reader.iter_messages(topics=[some_topic]):
        first_ns = ros_msg.log_time
        break

first_msg = None
with open(mcap_path, "rb") as mcap_f:
    reader = make_reader(mcap_f, decoder_factories=[DecoderFactory()])
    for _, _, _, ros_msg in reader.iter_decoded_messages(
        topics=[some_topic], start_time=first_ns, end_time=first_ns
    ):
        first_msg = ros_msg
        break

# Error because first_msg is None.
print(first_msg.header)

with open(mcap_path, "rb") as mcap_f:
    reader = make_reader(mcap_f, decoder_factories=[DecoderFactory()])
    for _, _, _, ros_msg in reader.iter_decoded_messages(
        topics=[some_topic], start_time=first_ns, end_time=first_ns + 1
    ):
        first_msg = ros_msg
        break

# Correct message.
print(first_msg.header)

Expected Behavior

According to the documentation here, messages that were logged either before start_time or after end_time aren't returned by the iterator, but the iterator appears to also exclude messages that were logged at the same time as end_time.

[linear]

FG-9583 end_time excludes messages that occur at the same time

[jtbandes]

Hmm, could it be as simple as changing these >= to >?

mcap/python/mcap/mcap/reader.py

Lines 314 to 315 in c8afa1e

	if end_time is not None and record.log_time >= end_time:
	continue

mcap/python/mcap/mcap/reader.py

Lines 93 to 94 in c8afa1e

	if end_time is not None and chunk_index.message_start_time >= end_time:
	continue

mcap/python/mcap/mcap/reader.py

Lines 527 to 528 in c8afa1e

	if end_time is not None and record.log_time >= end_time:
	continue

And I guess the question is whether the API was really intended to be inclusive or exclusive...

[exogen]

And I guess the question is whether the API was really intended to be inclusive or exclusive...

Change looks simple but yeah, the unfortunate part of making it inclusive (even though it matches the docs) is that you can't use an end_time as the next start_time if you want to iterate in time slices, since that'd duplicate records, so you'd need to do the +1ns trick. If it were up to me, I'd keep end time exclusive and update the docs, especially since it wouldn't be a breaking change to the library.

[indirectlylit]

I don't know how relevant it is here, but for API queries we made the decision to make end time inclusive rather than exclusive. The justification is documented in https://github.com/foxglove/app/pull/5868 but in summary:

• It makes API requests align well with timestamped sensor data samples, which is the primary use-case
• We offer offset-based pagination in the API, which means users don’t need half-open [) intervals to implement their own timestamp-based pagination client-side
• Other API endpoints all used closed [] intervals

[rdumas01]

Purely based on Python's usual behavior with bounds, I would also lean toward simply updating the docs (list[start:end], array[start:end], range(start, end), numpy.arange(start, end), etc... are all exclusive)