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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update MapAction layers and POC report #56

Merged
merged 14 commits into from
Jun 28, 2021
Merged

Conversation

ddebarros-mapaction
Copy link
Contributor

@ddebarros-mapaction ddebarros-mapaction commented Jun 11, 2021

Please add the type of change as label. If your PR is not ready for review and merge, please add 馃毀 to the PR title.

Description

The main change concerns the update of Map Action layers filters. We also changed the MapActionPoc report to be more consistent with this new layer definition.

Corresponding issue

Closes #43

New or changed dependencies

Checklist

  • I have updated my branch to main (e.g. through git rebase main)
  • My code follows the style guide and was checked with pre-commit before committing
  • I have commented my code
  • I have added sufficient unit and integration tests
  • I have updated the CHANGELOG.md

Please check all finished tasks. If some tasks do not apply to your PR, please cross their text out (by using ~...~) and remove their checkboxes.

@ddebarros-mapaction
Copy link
Contributor Author

Not sure if I should add this modification to the CHANGELOG

@matthiasschaub
Copy link
Collaborator

@ddebarros-mapaction I have a general question about this PR: What is the reason to include the OSM type relations in the filter query?
I ask because this can lead to performance issue on the ohsome API side.

The share of objects of relation elements with the tag highway is very low (https://taginfo.openstreetmap.org/keys/highway). Also the OSM Wiki states that the element relation should not be used with the tag highway. The same is in part true for the other keys. Although there are often more objects of type relation.

On the syntax part, I would remove the whitespace around the operators such as the equal sign (https://docs.ohsome.org/ohsome-api/stable/filter.html).

You can include this in the CHANGELOG.md. Just put in the PR title and link to it.

Otherwise this looks good to me!

@ddebarros-mapaction
Copy link
Contributor Author

Hello @matthiasschaub , thanks for the review. Just updated the two files (layer_definitions and CHANGELOG) with your recommendations.

Indeed, the type:relation is not indispensable. I first added it because some big lakes are represented with this type so I added it to be exhaustive. As this happened for the other layers as well I added everywhere in addition to type:way.

In any case, we can live without it and it won't affect the quality report significantly.

matthiasschaub
matthiasschaub previously approved these changes Jun 22, 2021
matthiasschaub
matthiasschaub previously approved these changes Jun 22, 2021
@ddebarros-mapaction
Copy link
Contributor Author

Hello @matthiasschaub, just saw that there was a conflict concerning the CHANGELOG file. Let me know if it's OK now.

matthiasschaub
matthiasschaub previously approved these changes Jun 28, 2021
matthiasschaub
matthiasschaub previously approved these changes Jun 28, 2021
@ddebarros-mapaction
Copy link
Contributor Author

a new change on the main branch CHANGELOG was made. I had to resolve the conflict :/

matthiasschaub
matthiasschaub previously approved these changes Jun 28, 2021
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
Co-authored-by: Johannes Visintini <johannes.visintini@heigit.org>
matthiasschaub
matthiasschaub previously approved these changes Jun 28, 2021
Co-authored-by: Johannes Visintini <johannes.visintini@heigit.org>
@joker234 joker234 self-requested a review June 28, 2021 09:59
@matthiasschaub matthiasschaub dismissed joker234鈥檚 stale review June 28, 2021 10:05

Minor fix already reviewed

@matthiasschaub
Copy link
Collaborator

@ddebarros-mapaction Yes this happens a lot to me as well with small changes after a review or merge with the main branch. I am going ahead an merge this branch now. Cheers 馃憤

@matthiasschaub matthiasschaub merged commit ea921dd into main Jun 28, 2021
@matthiasschaub matthiasschaub deleted the update-mapaction-layers branch June 28, 2021 10:10
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.

Update MapAction layers definition
3 participants