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

bugfix and simpler code #203

Merged
merged 7 commits into from
Jun 6, 2020
Merged

bugfix and simpler code #203

merged 7 commits into from
Jun 6, 2020

Conversation

rikojacob
Copy link
Contributor

The original code of get_nearest_location(location) has the property that if 'location' is a point on the track, the point after 'location' on the track is returned (for that case 'not distance' is True because distance is 0.0).
There is a version that only adjusts for this behavior in the middle commit.
I consider the version using min(..,key=lambda ..) even more readable.

One could consider this a hotfix, so perhaps it should be a pull request on master. If you prefer this, I can prepare such a pull request. (I'll have to do it from a different fork because this one has the full merge of dev...)

@coveralls
Copy link

coveralls commented May 29, 2020

Coverage Status

Coverage increased (+0.9%) to 87.753% when pulling c808826 on rikojacob:dev into ac394b9 on tkrajina:dev.

@tkrajina
Copy link
Owner

tkrajina commented May 30, 2020

Thanks @rikojacob! Can you also provide a test? For example use one of the provided test tracks, loop through points and make sure get_nearest_location returns those same points.

PS. Don't worry about dev/master I'll merge it in master.

@rikojacob
Copy link
Contributor Author

Done, slightly differently, including some tests if None is handled as expected.
I also changed a type annotation in a separate commit.

@rikojacob
Copy link
Contributor Author

Now everything is based on walk(), no test fails, no type errors in new code.
I'll leave it for the moment.

Two more general comments:
In python, -1 is a bad default value for indices (like the track index in Segment.get_nearest..) because it can be used as an index referencing to the last entry.

Perhaps it would make sense to change NearestLocationData to LocationData and make it the type of elements walk() returns

@tkrajina tkrajina merged commit c808826 into tkrajina:dev Jun 6, 2020
@tkrajina
Copy link
Owner

tkrajina commented Jun 6, 2020

Merged, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants