Skip to content
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

DiscreteTrajectorySegment, part 4: downsampling #3150

Merged
merged 12 commits into from
Oct 12, 2021

Conversation

pleroy
Copy link
Member

@pleroy pleroy commented Oct 11, 2021

template<typename Frame>
void DiscreteTrajectorySegment<Frame>::SetDownsampling(
DownsamplingParameters const& downsampling_parameters) {
// TODO(phl): Do we need this precondition?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we have ClearDownsampling we might as well be able to change it directly (but the semantics of doing either are unclear if there is any actual downsampling going on, see my comment in the .h).


// Clear the downsampling parameters. From now on, all points appended to the
// segment are going to be retained.
void ClearDownsampling();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The semantics of setting and clearing the downsampling on a segment with more than one point are unclear, and the comments fail to clarify this.

Clearing the downsampling causes points to be retained, but what happens when you reset the downsampling (or when you set it on a trajectory which never had downsampling) ? Are those points stuck in their dense state forever, or does it commute with appending ? If they are stuck in their dense state forever, do we have a way to identify those points ?

I would recommend forbidding mutations of the downsampling parameters on a segment whose size is greater than 1, otherwise the semantics are going to be unwieldy to define and think about.

@eggrobin eggrobin added the LGTM label Oct 12, 2021
@pleroy pleroy merged commit f8fe5f4 into mockingbirdnest:Ondelette Oct 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants