Skip to content

Add support for direction tag into viewpoint pois#1916

Merged
nvkelso merged 6 commits intotilezen:masterfrom
rwrx:master
Aug 9, 2019
Merged

Add support for direction tag into viewpoint pois#1916
nvkelso merged 6 commits intotilezen:masterfrom
rwrx:master

Conversation

@rwrx
Copy link
Copy Markdown
Contributor

@rwrx rwrx commented Jul 8, 2019

Add support for direction tag into viewpoint pois.

Connects with #598.

Copy link
Copy Markdown
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
Copy Markdown
Member

nvkelso commented Jul 9, 2019

👀

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

nvkelso commented Jul 9, 2019

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

Copy link
Copy Markdown
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.

Comment thread yaml/pois.yaml Outdated
output:
<<: *output_properties
kind: viewpoint
direction: {col: direction}
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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