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

User mosaics #133

Merged
merged 2 commits into from
May 2, 2018
Merged

User mosaics #133

merged 2 commits into from
May 2, 2018

Conversation

mojodna
Copy link
Collaborator

@mojodna mojodna commented Apr 28, 2018

This is a refined version of #122 specific to users (and feels sufficient to deploy from my perspective).

It allows marblecutter-openaerialmap w/ https://github.com/mojodna/marblecutter-openaerialmap/blob/master/openaerialmap/web.py#L274-L292 to render tiles from /user/<id>/{z}/{x}/{y}.png.

It includes logic to prioritize sources that more fully cover a given tile (preferring recent + higher resolution versions), but that's currently disabled due to the Zanzibar data hitting w8r/martinez#74.

(This is the same logic we'll want for the global mosaic, especially since the likelihood of more sources fully covering a given tile is substantially higher.)

Copy link
Collaborator

@sharkinsspatial sharkinsspatial left a comment

Choose a reason for hiding this comment

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

If you can enable repository maintainer permissions I can add some tests so they can be all part of the same pr.

I will try to look into some other options for tile coverage calculation.

What are your thoughts about storing catalog as property of User that is updated when new images get uploaded so it is not recalculated for every request? Probably overkill for now but might be worth consideration in the future?

const { user } = request.params;

return Promise.all([
User.findOne({
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the user record necessary for the catalog json? Can we just use the user id then we can avoid the findOne query and just perform the Meta.find

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just to fetch the username for descriptive purposes (which is never surfaced anywhere public).

Meta.find({
user
}, {
bbox: 1, gsd: 1
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure but I think the idiomatic way to do projections with Mongoose is with string literals. http://mongoosejs.com/docs/api.html#find_find
I found the 1s confusing since I normally associate this with sort order.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I found the space-delimited string literals mildly alarming, so deferred to MongoDB docs. I'll update that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am ok with the object notation. Just wasn't sure what the standard way of doing it was.

}
}
}, {
acquisition_end: 1,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same comment as previously concerning projections.

@mojodna
Copy link
Collaborator Author

mojodna commented May 1, 2018

If you can enable repository maintainer permissions I can add some tests so they can be all part of the same pr.

Checkbox checked.

I will try to look into some other options for tile coverage calculation.

JSTS might work (it was replaced in turf by martinez), though footprints are occasionally MultiPolygons.

For the user mosaic, skipping the tile coverage calculation seems ok, which will buy us some time on waiting for a fix to that crasher. I'm not smart enough to understand what it's doing in the while loop I isolated.

What are your thoughts about storing catalog as property of User that is updated when new images get uploaded so it is not recalculated for every request? Probably overkill for now but might be worth consideration in the future?

Maybe? The catalog.json request is cached (@lru_cache) on each Lambda runner (and could be cached on the oam-api side). My bigger concern is the need to hit a tile endpoint for each tile to figure out what to display. Capping that on oam-api (over-zooming / meta tiling) / limiting it to alternate zooms (0, 2, 4, ...) might help at the expense of telling the tiler to fetch more sources than would be ideal.

Copy link
Collaborator

@sharkinsspatial sharkinsspatial left a comment

Choose a reason for hiding this comment

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

@smit1678 We can approve this pull request and I will add some tests with another pr.

@smit1678 smit1678 merged commit e3fcd44 into hotosm:develop May 2, 2018
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.

4 participants