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

Tiers for landuse #980

Closed
wants to merge 7 commits into from
Closed

Conversation

zerebubuth
Copy link
Member

Adds a property representing the "tier" from #473; an integer from 1-6.

Connects to #473.

@rmarianski could you review, please?

@rmarianski
Copy link
Member

👍

@@ -1 +1,72 @@
kind,scalerank
kind,operator,zoom::int,area::int,scalerank
national_park;park or protected land;battlefield,!United States Forest Service,3,>=300000000,1
Copy link
Member

Choose a reason for hiding this comment

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

  1. park or protected land is an outmoded v0.10 reference, it should be removed I think?
  2. Make sure !United States Forest Service matches Olga's change from last week?

@nvkelso
Copy link
Member

nvkelso commented Aug 20, 2016

Looks like this is written more for tier than scalerank?

What about adding an additional column for scalerank for the different area brackets (more on a 1 to 10 scale?) so features within the same kind would get different scalerank values based on their areas (my additional proposal), but all be in the same tier based on the kind (as currently written in the PR).

In the stylesheet it would be nice to be able to filter generically on tier and scalerank for most landuse features.

CSVMatcher now uses the header key to determine if the column is an
output column - those with the prefix "output:" are used. This
allows the use of multiple outputs in the same CSV, and that is used
to output both the scalerank and tier from the landuse CSV.
@zerebubuth
Copy link
Member Author

@rmarianski I've changed the code somewhat to allow CSVs to have multiple output columns. Please could you take a look and let me know what you think?

@nvkelso Please look at the test and let me know if that's what you wanted from tier and scalerank.

@rmarianski
Copy link
Member

👍

… clearer. Also removed unnecessary 'sys' import left over from debugging.
@zerebubuth zerebubuth assigned nvkelso and unassigned zerebubuth Aug 23, 2016
# for the first row where all the matchers return true.
target_vals = {}

for i in range(0, len(row)):
Copy link
Member

Choose a reason for hiding this comment

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

range defaults to 0, but is this equivalent?

for (is_target, key, typ), val in zip(row, cols):

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes! That's much better, thanks. Fixed in 1cf7672.

@nvkelso nvkelso assigned zerebubuth and unassigned nvkelso Aug 24, 2016
@nvkelso
Copy link
Member

nvkelso commented Aug 24, 2016

Tests are looking good. But one question, what is the 2nd test on

{ 'kind': 'national_park', 'id': -921675, 'tier': 1,
'scalerank': 8 })
assert_has_feature(
7, 28, 57, 'landuse',
{ 'kind': 'national_park', 'id': -921675,
'tier': type(None), 'scalerank': type(None) })
meant to do? Test that the feature not be included at the lower zoom or that it not receive the tier and scalerank?

That US National Forest thing is definitely not a national_park ;)

@nvkelso
Copy link
Member

nvkelso commented Aug 24, 2016

@zerebubuth
Copy link
Member Author

But one question, what is the 2nd test meant to do? Test that the feature not be included at the lower zoom or that it not receive the tier and scalerank?

The latter - I thought that's what this issue was about: Providing a tier and scalerank on features that we were being opinionated about for our styles, but not changing the actual min_zoom?

Therefore the features which we don't want to display at a particular zoom are still present in the tiles if someone else wants to show them earlier. I wasn't entirely sure based on the description in #473. It would be redundant if min_zoom == scalerank, so I thought it must have a different meaning.

Please let me know if you wanted to alter the actual min_zoom values, as that would be a completely different implementation.

That US National Forest thing is definitely not a national_park ;)

Tracking in #987.

@zerebubuth
Copy link
Member Author

Following on from discussions about protect_class, I'm having trouble understanding what the desired behaviour is. It would be really helpful if we could pick out some concrete examples of things having the different properties which should and should not get assigned to the different tiers.

@zerebubuth zerebubuth changed the title Scalerank for landuse Tiers for landuse Aug 24, 2016
@zerebubuth
Copy link
Member Author

Closing this as per discussion. There will be a new PR implementing this by changing the min zoom rules in the YAML and applying the area filters and tier values there.

@zerebubuth zerebubuth closed this Aug 24, 2016
@zerebubuth zerebubuth deleted the zerebubuth/473-landuse-scalerank branch September 16, 2016 14:27
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