Skip to content

Tiers for landuse#980

Closed
zerebubuth wants to merge 7 commits intomasterfrom
zerebubuth/473-landuse-scalerank
Closed

Tiers for landuse#980
zerebubuth wants to merge 7 commits intomasterfrom
zerebubuth/473-landuse-scalerank

Conversation

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

👍

Comment thread spreadsheets/scalerank/landuse.csv Outdated
@@ -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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

👍

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

for i in range(0, len(row)):
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.

range defaults to 0, but is this equivalent?

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

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

nvkelso commented Aug 24, 2016

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