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

upgrade OSHDB to version 0.6 #86

Merged
merged 18 commits into from
Jan 13, 2021
Merged

upgrade OSHDB to version 0.6 #86

merged 18 commits into from
Jan 13, 2021

Conversation

tyrasd
Copy link
Member

@tyrasd tyrasd commented Dec 11, 2020

see https://github.com/GIScience/oshdb/blob/master/CHANGELOG.md#060

  • "fix" tests after upstream changes:

    improve accuracy of built-in geometry helper functions which calculate the geodesic lengths and areas of OSM geometries

  • double-check new integrated filter implementation
  • add to changelog

@tyrasd tyrasd added the dependencies Pull requests that update a dependency file label Dec 11, 2020
@tyrasd tyrasd added this to the 1.3 milestone Dec 11, 2020
@tyrasd tyrasd requested a review from FabiKo117 December 11, 2020 13:46
@joker234
Copy link
Member

@tyrasd you could update the ohsome parent as well ;)

@tyrasd
Copy link
Member Author

tyrasd commented Dec 11, 2020

@tyrasd you could update the ohsome parent as well ;)

good point, done in c5b9f56

@tyrasd
Copy link
Member Author

tyrasd commented Dec 16, 2020

@bonaparten @FabiKo117 I quickly checked the failing assertations from https://jenkins.ohsome.org/job/ohsome-api/view/change-requests/job/PR-86/3/consoleFull and found all "new" results to be within a 0.35% (relative) margin of the "old" test-values. Which is what I would have expected from the changes in GIScience/oshdb#193. Did I overlook some cases where the discrepancy is larger?

@FabiKo117
Copy link
Contributor

@bonaparten @FabiKo117 I quickly checked the failing assertations from https://jenkins.ohsome.org/job/ohsome-api/view/change-requests/job/PR-86/3/consoleFull and found all "new" results to be within a 0.35% (relative) margin of the "old" test-values. Which is what I would have expected from the changes in GIScience/oshdb#193. Did I overlook some cases where the discrepancy is larger?

@tyrasd No, it seems like this is the maximum discrepancy then. Probably I was thinking of the higher differences in bigger values, when I was referring to differenes in the lower percentage area when mentioning it in the meeting. I'd suggest to use a delta of 0.5 in the tests + adapting the current values to the more accurate ones. What do you think about that?

Another thing: all the /contribution tests are now failing for the following reason:
groupByEntity() must be called before any `map` or `flatMap` transformation functions have been set
The first line in the ohsome API code that is referenced in the error message is this one.

@tyrasd
Copy link
Member Author

tyrasd commented Dec 18, 2020

I'd suggest to use a delta of 0.5 in the tests + adapting the current values to the more accurate ones. What do you think about that?

that sound good 👍

Another thing: all the /contribution tests are now failing

oh, true. let me debug that.

@tyrasd tyrasd self-assigned this Dec 18, 2020
@tyrasd tyrasd changed the title upgrade OSHDB to version 0.6.0 WIP: upgrade OSHDB to version 0.6.0 Dec 18, 2020
@tyrasd
Copy link
Member Author

tyrasd commented Dec 18, 2020

Another thing: all the /contribution tests are now failing

oh, true. let me debug that.

Quick update: I run into the following bug while trying to fix this: GIScience/oshdb#321 😬

@tyrasd tyrasd changed the title WIP: upgrade OSHDB to version 0.6.0 🚧 upgrade OSHDB to version 0.6.0 Dec 18, 2020
@tyrasd
Copy link
Member Author

tyrasd commented Dec 23, 2020

Another thing: all the /contribution tests are now failing

oh, true. let me debug that.

Quick update: I run into the following bug while trying to fix this: GIScience/oshdb#321

this should now hopefully be fixed with oshdb version 0.6.1 (updated this PR with 6e66a5e) 🤞

@tyrasd tyrasd changed the title 🚧 upgrade OSHDB to version 0.6.0 upgrade OSHDB to version 0.6.0 Jan 4, 2021
@tyrasd tyrasd changed the title upgrade OSHDB to version 0.6.0 upgrade OSHDB to version 0.6 Jan 4, 2021
@bonaparten
Copy link
Contributor

Tests are now updated with new values and delta.

Copy link
Contributor

@FabiKo117 FabiKo117 left a comment

Choose a reason for hiding this comment

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

Please look at the detailled review comments. There are several unused import statements from removed code parts, make sure to check again the changed classes and organize the imports there again (think mostly in classed edited by @tyrasd).

About the tests: I did not check every single test, but individual ones without the delta and they worked fine and all tests are running through of course. Please check the review comment about commenting @bonaparten.

README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@FabiKo117 FabiKo117 left a comment

Choose a reason for hiding this comment

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

See inline comments.

@tyrasd tyrasd requested a review from FabiKo117 January 13, 2021 09:39
Copy link
Contributor

@FabiKo117 FabiKo117 left a comment

Choose a reason for hiding this comment

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

Looks good now. Marking the test values as 'final' also makes sense here.

@joker234
Copy link
Member

Looks good now. Marking the test values as 'final' also makes sense here.

I checked 3d3ec09 against a test cluster with oshdb version 0.6.1 using our usual tools and couldn't find anything suspicious.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants