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

get_time_diffs and get_distances inconsistent #57

Open
blacknapkinsuk opened this issue Apr 1, 2021 · 4 comments · May be fixed by #59
Open

get_time_diffs and get_distances inconsistent #57

blacknapkinsuk opened this issue Apr 1, 2021 · 4 comments · May be fixed by #59
Assignees
Labels
bug Something isn't working

Comments

@blacknapkinsuk
Copy link

For N timestamps, get_time_diffs returns n-1 results. Should this be N, with the first value being 0?
For N positions, get_distances returns N results. Neither the first or last distance is 0. One of these should be incorrect.
Both methods should be consistent.

Calculation appears to be difference between N and N +1, hence It appears that the last distance calculation must be incorrect as there is no N + 1

However, a change in behaviour will break existing code, so more clarification in the API documentation is perhaps called for.

@aniketmitra001 aniketmitra001 self-assigned this Apr 3, 2021
@aniketmitra001 aniketmitra001 added the bug Something isn't working label Apr 3, 2021
@aniketmitra001 aniketmitra001 added this to To do in Release v1.0 via automation Apr 3, 2021
@aniketmitra001
Copy link
Collaborator

@blacknapkinsuk Please see the discussion for Issue #51 .
The initial idea was that both get_time_diffs and get_distances should return N values , however as @bacusters suggested there should be an option to return (n-1) values for n inputs where the idea is that time_diff and distance are attributes to the edges of the polyline and not necessarily attributes of the points of the polyline.

I have opened a pull request hotfix-57 and fixed and overloaded the get_time_diffs() and get_distance() functions to accept and initial value and therefore return N results for an input of size N.

https://github.com/heremaps/movetk/blob/932885a982d0ef0c65250e46fbae2ab084afb49e/src/include/movetk/utils/TrajectoryUtils.h#L159

https://github.com/heremaps/movetk/blob/932885a982d0ef0c65250e46fbae2ab084afb49e/src/include/movetk/utils/TrajectoryUtils.h#L181

Can you please review the changes and see if this makes sense?

@aniketmitra001 aniketmitra001 linked a pull request Apr 5, 2021 that will close this issue
@aniketmitra001
Copy link
Collaborator

@blacknapkinsuk , the same had also been fixed by @bacusters in the python-binding branch

This hasn't been merged with master yet, we need to check which of the two solutions is better and incorporate that. Any preferences?

@blacknapkinsuk
Copy link
Author

Looking at the change, I am unsure why this code changes the behaviour. Surely the initial value is just overwritten after being set? I do not see the iterator being advanced after setting the initial value.

@aniketmitra001
Copy link
Collaborator

@blacknapkinsuk the OutputIterator is of type movetk_core:: movetk_back_insert_iterator
The assignment operator calls the push_back() modifier on the container and therefore increments the iterator at the same time.
So no explicit increment of the iterator is need as the assignment step "appends and increments" in one go

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Release v1.0
  
To do
Development

Successfully merging a pull request may close this issue.

2 participants