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

Make locality thinning grids smaller #2040

Merged
merged 10 commits into from Jan 28, 2022
Merged

Make locality thinning grids smaller #2040

merged 10 commits into from Jan 28, 2022

Conversation

iandees
Copy link
Member

@iandees iandees commented Jan 3, 2022

Extending #2001 with a few tweaks to the queries.yaml arguments for the locality thinning.

The grid_width: 3 was way too small, resulting in way too few localities showing up:

image

Changing to grid_width: 10 results in a more reasonable number of localities:

image

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.

If this is a 512 px tile size versus 256 issue, then should we try doubling the grid_width instead? Your example from 3 > 10 looks far too dense to me.

queries.yaml Outdated Show resolved Hide resolved
queries.yaml Outdated Show resolved Hide resolved
@iandees
Copy link
Member Author

iandees commented Jan 4, 2022

Sounds good. I was trying to roughly match the styled map from before thinning (where the client was doing all the thinning), and a plain doubling seemed to be missing some of the smaller localities. But here's a new round of screenshots with plain doubling:

  Before thinning This PR as of e0fe150
z7 image image
z8 image image
z9 image image

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.

Looking good, let's see it in a build :)

@nvkelso
Copy link
Member

nvkelso commented Jan 5, 2022

Build says the numbers still aren't right.

If Tilequeue is operating on a larger window (like 4x4 grid of requested tiles), then we should we instead apply multiplier to vector-datasource for an allowlisted list of config props values (like also for kind_tile_rank)?

queries.yaml Outdated Show resolved Hide resolved
queries.yaml Outdated Show resolved Hide resolved
queries.yaml Outdated Show resolved Hide resolved
queries.yaml Show resolved Hide resolved
@nvkelso nvkelso added this to the v1.9.0 milestone Jan 7, 2022
@iandees
Copy link
Member Author

iandees commented Jan 7, 2022

Sounds like we're happy with this most recent change. So last couple things here before we merge:

  • Fix integration test 1999
  • Merge the first two queries.yaml sections using gridded thinning function (for zooms z8-z9 and z9-z11) into a single range, since their values are the same

@nvkelso
Copy link
Member

nvkelso commented Jan 7, 2022

We also need to fix the config and logic so it works for both Tilequeue and Tileserver, please.

@iandees
Copy link
Member Author

iandees commented Jan 10, 2022

We also need to fix the config and logic so it works for both Tilequeue and Tileserver, please.

I haven't found a path to do that yet. I'll keep thinking.

@iandees
Copy link
Member Author

iandees commented Jan 10, 2022

After talking with Travis for a bit this afternoon we weren't able to find an approach that handles both tileserver and tilequeue. I'm going to spend the rest of the evening trying to get tilequeue meta-tile running locally so I can do some debugging and maybe find something we're missing.

The sticking point is that metatile-based builds run the functions in queries.yaml (like grid thinning) once for all the data in the metatile and the functions only get the nominal_zoom to work from. For metatile builds, that is the zoom level of the metatile that is being built, for tileserver-based requests that is the zoom level of the tile being requested (including adjustments for 512 or 256-sized tiles).

queries.yaml Outdated
@@ -1052,7 +1052,7 @@ post_process:
end_zoom: 9
items_matching: { kind: locality }
max_items: 1
grid_width: 3
grid_width: 52181.0113
Copy link
Member

Choose a reason for hiding this comment

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

It's not obvious this is in meters... should the param name be grid_width_meters?

Is the fractional meters necessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

I suppose they're not necessary, but using fractions means we get closer to exactly 3x3 subdivisions in a single 256px tile at this zoom level. If we go to integer values here, then the tiny subdivisions at the edge of the tile are slightly bigger and are more likely to get an extra feature in there.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed to grid_width_meters in 2eca280.

@@ -3189,8 +3183,8 @@ def keep_n_features_gridded(ctx):
# Calculate the bucket to put this feature in.
# Note that this purposefully allows for buckets outside the unpadded bounds
# so we can bucketize the padding area, too.
bucket_x = int((shape.x - minx) / bucket_width)
bucket_y = int((shape.y - miny) / bucket_height)
bucket_x = int((shape.x - minx) / grid_width)
Copy link
Member

Choose a reason for hiding this comment

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

Is this already flexible to whole number grid_width meters? It's rounding it to an int, so maybe? If it's already padded then seems like it's fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's rounding to an int after doing floating point math.

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.

Two nits but otherwise LGTM and works in a QA build.

@iandees iandees merged commit dba56b9 into master Jan 28, 2022
@iandees iandees deleted the dees/tweak-thinning branch January 28, 2022 18:59
iandees added a commit that referenced this pull request Jan 29, 2022
iandees added a commit that referenced this pull request Jan 29, 2022
Fix keep_n_gridded tests after changes in #2040
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.

None yet

2 participants