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

Add support for direction tag into viewpoint pois #1916

Merged
merged 6 commits into from
Aug 9, 2019

Conversation

rwrx
Copy link
Contributor

@rwrx rwrx commented Jul 8, 2019

Add support for direction tag into viewpoint pois.

Connects with #598.

Copy link
Member

@rmarianski rmarianski left a comment

Choose a reason for hiding this comment

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

Thanks! This seems fine to me, but deferring to @nvkelso if this is cool to add in.

Mind adding a quick test for this? You should be able to copy an existing one from the integration-test directory.

@nvkelso
Copy link
Member

nvkelso commented Jul 9, 2019

👀

@nvkelso nvkelso self-requested a review July 9, 2019 17:17
@nvkelso
Copy link
Member

nvkelso commented Jul 9, 2019

This connects with #598 in the v1.9.0 milestone.

Copy link
Member

@nvkelso nvkelso left a comment

Choose a reason for hiding this comment

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

Thanks for tackling this! Please update it as detailed below and we can get this merged ASAP :)

Because direction is sometimes not provided as a numerical degree (eg a value of 90 meaning 90°), like E for "east" = 90 and so on as detailed #598 (comment), we'll also need to add a new vectordatasource transform in Python to ensure values are in numerical range of 0-359 after also converting valid string input to numerical values. If the input isn't valid we should drop the property.

An existing transform to model off is vectordatasource.transform.height_to_meters or vectordatasource.transform.pois_capacity_int.

Note there's also a helper function to _to_float_meters that we'd also need to duplicate (and call something like _to_int_degrees (we could do float, but if so we'd only want to the tenths place?).

Once the transform Python code is added, we also need to trigger it by adding new line after:

So every feature in the POIs layer has the transform applied.

Then we'd need to add a test like Rob suggested... and make sure the test file tested for valid numerical, out of range numerical, and E > 90 conversions.

yaml/pois.yaml Outdated
@@ -2206,6 +2206,7 @@ filters:
output:
<<: *output_properties
kind: viewpoint
direction: {col: direction}
Copy link
Member

Choose a reason for hiding this comment

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

There are many more POIs that include direction, so let's move this to the very top of the YAML doc after wikidata_id: {col: wikidata} in the

global:
  - &output_properties:

section, please.

@rwrx
Copy link
Contributor Author

rwrx commented Jul 29, 2019

@nvkelso I am sorry for this late answer, I was quite busy and it is also summer time :). I have tried to implement it as you wrote, but I am not sure if I understand it correctly, so if needed I can make further changes. Also could I ask you for advise how to write tests for this?

@nvkelso
Copy link
Member

nvkelso commented Jul 31, 2019

This looks good :) I'll have a look this week and add the tests, thank you!

@nvkelso
Copy link
Member

nvkelso commented Aug 9, 2019

Going to merge this and add tests in new PR.

@nvkelso nvkelso merged commit ee352fd into tilezen:master Aug 9, 2019
@nvkelso nvkelso mentioned this pull request Aug 10, 2019
2 tasks
@nvkelso
Copy link
Member

nvkelso commented Aug 12, 2019

Fixed a few edge case bugs and added tests, docs in #1921.

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