-
Notifications
You must be signed in to change notification settings - Fork 126
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Abstract perception eval operator #168
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the change. Please see my comments to ensure the naming style is consistent with the Pylot code base.
Addressed comments, and pushing change to deal with missing start times |
assert st == game_time, 'Incorrect frontier' | ||
self._accuracy_compute_buffer.append((st, et, False)) | ||
index = self._start_time_frontier | ||
on_new_prediction = self._start_time_frontier == len( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using len(self._prediction_start_end_times) might be buggy because we don't have any guarantee that we're not going to have any entries for timestamps t + 1, t + 2.. in self._prediction_start_end_times when the watermark callback for timestamp t is invoked.
Do we need to deal with the missing start times here? or should we just assume that these are the responsibility of the upstream operator. IMO, we should let the upstream operators decide what the policy is (e.g.,output previous detections, yada yada).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch.
As for the second part, that is what the original sequencer was doing. As it stands this'd require additions to the sequencer as well as at the tracker down the line once it skips frames, creating a common function implemented in multiple places. This makes me inclined to do it in this module.
I pushed the changes to address the new comments, and they pass the tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
No description provided.