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

rio-tiler 2.0 #154

Merged
merged 29 commits into from
May 18, 2020
Merged

rio-tiler 2.0 #154

merged 29 commits into from
May 18, 2020

Conversation

vincentsarago
Copy link
Member

@vincentsarago vincentsarago commented Mar 10, 2020

rio-tiler Version 2 🎉

closes #152 #144 #139 #124 #105 #150 #145

@vincentsarago
Copy link
Member Author

This will be a major release with a lot of breaking changes. I'll write up an issue explaining how we will release (alpha, beta ... ) and how to switch to this new version.

CHANGES.txt Outdated Show resolved Hide resolved
vincentsarago and others added 10 commits March 10, 2020 15:33
Co-Authored-By: jqtrde <hi@jacquestardie.org>
* set padding default to 0

* switch to terracotta "calculate_default_transform"
* add more metadata info

* add missing test

* update changes [skip-ci]
* 4bands colormap and new utility functions

* update old cmaps and add tests
* remove alpha band from output data array

* update changelog [skip ci]
@vincentsarago vincentsarago marked this pull request as ready for review March 19, 2020 20:35
@vincentsarago vincentsarago changed the title v2 sketch rio-tiler 2.0 Mar 19, 2020
* fall back to gdal calculate_default_transform for dateline separation crossing dataset

* release note
@kylebarron
Copy link
Member

Is there anything else you need help with on 2.0?

I see 2.0 brings support for partial reads, so I'm thinking of trying to adapt rio-tiler-mosaic/cogeo-mosaic-tiler to work with rio-tiler 2.0 and see how it goes and if I can improve performance.

@vincentsarago
Copy link
Member Author

thanks @kylebarron , really appreciated.

I don't think it will change much to rio-tiler-mosaic because it takes .tile function as input. but in cogeo-mosaic-tiler we will need to change from rio_tiler.main import tile as cogeoTiler by from rio_tiler.io.cogeo import tile as cogeoTiler`.

to support part in rio-tiler-mosaic we could just add a mosaic_part function that takes rio-tiler.reader.part like function as input (there is only one right now but we might add another one to support expression`.

to support change in cogeo-mosaic-tiler or cogeo-tiler I'll also need to update https://github.com/remotePixel/cogeo-layer

if not bounds_crs:
bounds_crs = dst_crs

bounds = transform_bounds(bounds_crs, dst_crs, *bounds, densify_pts=21)
Copy link
Member Author

Choose a reason for hiding this comment

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

following @kylebarron comment. I guess we should also make this optional, and only use transform_bounds if bounds_crs != dst_crs ?

Copy link
Member

@kylebarron kylebarron Mar 23, 2020

Choose a reason for hiding this comment

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

Actually, to be clearer, I found that when bounds_crs == dst_crs, it only took ~1ms, it's only when bounds_crs != dst_crs that it took 85ms.

In current _tile_read, transform_bounds is called twice, where the second is reprojecting to mercator coordinates, and you can see it's only the second that's slow
image

Copy link
Member Author

Choose a reason for hiding this comment

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

Excellent, so we can keep it as it is then.

Can I ask how are you profiling the function?

Copy link
Member

Choose a reason for hiding this comment

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

I'm using AWS X-Ray, and using their Python SDK to define manual timing blocks.

I'm timing parts of the underlying libraries by shadowing their function definitions, i.e.

# local import
from cogeo_mosaic_tiler.temp import (
    fetch_and_find_assets
)

and have a file temp.py locally that's essentially the same as the original file, but with

xray_recorder.begin_subsegment('name')
...
xray_recorder.end_subsegment()

interspersed throughout. E.g.

def _aws_get_data(key, bucket, client: boto3_session.client = None) -> BinaryIO:
    if not client:
        xray_recorder.begin_subsegment('make session')
        session = boto3_session()
        xray_recorder.end_subsegment()

        xray_recorder.begin_subsegment('make client')
        client = session.client("s3")
        xray_recorder.end_subsegment()

    xray_recorder.begin_subsegment('client.get_object')
    response = client.get_object(Bucket=bucket, Key=key)
    xray_recorder.end_subsegment()
    return response["Body"].read()

Then make a new lambda package, deploy, pan the web map a little, and go into the AWS console to see the trace output.

I have a couple screenshots in issues here and here (the latter didn't profile within mosaic_tiler)

@kylebarron
Copy link
Member

Cool, I'll see how it goes.

As an aside, for a newcomer I've found it a little confusing keeping track of what each repository does. Each project handles a subtly distinct need, but they're so intertwined that it's hard to keep them straight.

rio-tiler
rio-cogeo
rio-tiler-mosaic
cogeo-mosaic
cogeo-mosaic-tiler

I don't really have a suggestion for how things could be clearer, but just something to keep in mind.

@vincentsarago
Copy link
Member Author

thanks @kylebarron

there are a lot of repo (split in mainly 2 orgs) which could hardly be simplified (maybe we should move everything to cogeotiff ?)

I'll think about this any maybe create an awesome-cog repo/doc ;-)

name org description
rio-tiler cogeotiff Core module to read data from a COG
rio-cogeo cogeotiff CLI + python module to easily create COG
rio-tiler-mosaic cogeotiff rio-tiler plugin to handler multiple overlapping dataset (was made as a separate module to keep rio-tiler simple)
rio-tiler-mvt cogeotiff rio-tiler plugin to create vector tiles from a numpy array
rio-tiler-crs cogeotiff rio-tiler plugin to create tile in other CRS
--- --- ---
cogeo-mosaic devseed CLI+python module to create MosaicJSON (not linked directly to rio-tiler nor rio-cogeo)
cogeo-mosaic-tiler devseed AWS stack - Dynamic tiler for mosaics.
cogeo-tiler devseed AWS stack - Dynamic tiler for simple COG.
titiler devseed FastAPI stack for dynamic tiling
--- --- ---
rio-viz devseed visualize COG in browser
--- --- ---
cogeo-watchbot devseed AWS stack - Create COG + MosaicJSON at scale
cogeo-watchbot-light devseed AWS stack - Create COG at scale
cogeo-watchbot-fargate devseed AWS stack - Create COG at scale using ECS fargate

@kylebarron
Copy link
Member

kylebarron commented Mar 23, 2020

maybe create an awesome-cog

I think that would be awesome and really helpful, just to have a description of all the repositories in one place.

If you're going to make an awesome-cog, I'd also add rio-glui, and there are probably more Mapbox COG repositories

rio_tiler/utils.py Outdated Show resolved Hide resolved
docs/v2_migration.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
vincentsarago and others added 7 commits April 21, 2020 02:37
* revert float32 landsat output

* update changelog [skip ci]
* Add colormap thumbnail script

* Add colormaps doc

* Coerce name to lower case

* Apply black on data

* Escape spaces in markdown

* Replace space with underscore

* Add docstring

* rename geographic colormaps

* add refs for custom colormaps

* fix link

* format

Co-authored-by: Kyle Barron <barronk@mit.edu>
…ction in io.cogeo (#182)

* allow reading part of data at hight resolution and add area/point function in io.cogeo

* make flake8 happy

* update change log [skip ci]

* remove print statement [skip ci]
indexes: list of ints or a single int, optional
Band indexes
out_window: rasterio.windows.Window, optional DEPRECATED
Output window to read.
Copy link
Member

Choose a reason for hiding this comment

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

I think you should take out deprecations before the final 2.0 release

Copy link
Member Author

@vincentsarago vincentsarago May 18, 2020

Choose a reason for hiding this comment

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

yes that's the plan. If we do a Beta release, all the depreciation warning will go

@vincentsarago vincentsarago merged commit 3eea54d into master May 18, 2020
@vincentsarago vincentsarago deleted the v2 branch June 18, 2020 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pass (height, width) to "rio_tiler.utils._tile_read" instead of tile size
3 participants