OSM tiling #440
OSM tiling #440
Conversation
There was a problem hiding this comment.
Thanks for putting this together @B-Dalmijn! Some initial comments
- I've tried testing it with a local dataset in a project UTM CRS but than the dres after transforming (line 2305) becomes negative and the I get an error on the next line.
- Could you add some more comments to explain what certain blocks of code are doing, For instance I don't understand what lines 2304-2306 are for.
- out of curiosity, would it be possible to reuse (part of) to_xyz_tiles after you have created the first zoom level (close to the native data resolution) for the subsequent zoom levels?
Let me know if something is unclear.
|
@DirkEilander Your last point (copying from 'to_xyz_tiles') is one I thought of for a bit when I can coding. However I really need to have the locations of the tiles (windows basically) as they are fixed. That meant I could not translate the code from 'to_xyz_tiles' one to one, where this is not used whatsoever. Maybe talking in person I can explain more clearly. |
|
No clue why Sonarcloud failed where it did... |
There was a problem hiding this comment.
I can't totally attest to the semantics of the code, but assuming that's okay I'd say this looks okay. I left a few comments regarding code quality and possibilities for optimizing memory if that is an issue. Should still wait for Dirks approval to make sure the calculations are actually correct.
|
I processed the comments, reshuffled stuff a bit and made it more readable (I think). |
There was a problem hiding this comment.
I don't feel I can say anything meaningful about the maths without taking a significant amount of time to understand it. The code looks good, with the comment that the mercantile package seems to not be actively maintained. if it is mostly because it's considered stable, (which I don't feel qualified to comment on) then it's okay, but otherwise it might be good if we can find a suitably maintained alternative. Other than that LGTM but I'll leave the final approval for Dirk
|
@savente93 It's designated as a stable release. I'd say it is indeed considered stable and just finished. |
There was a problem hiding this comment.
Thanks for the last updates @dalmijn! A few very small final changes left before merging. Can you apply these and merge?
hydromt/raster.py
Outdated
| # Setting up information for zoomlevel calculation and | ||
| # determination of tile windows | ||
| # This section is purely for the resolution |
There was a problem hiding this comment.
Could this section be moved inside the if max_lvl is None: statement?
hydromt/raster.py
Outdated
| @@ -2292,6 +2292,13 @@ def to_slippy_tiles( | |||
| If None, the zoomlevels will be determined based on the data resolution | |||
| driver : str, optional | |||
| file output driver, one of 'png', 'netcdf4' or 'GTiff' | |||
| cmap : str | object, optional | |||
| A colormap, either defined by a string and imported from matplotlib | |||
| via that string or as a ListedColormap object from matplotlib itself. | |||
There was a problem hiding this comment.
| via that string or as a ListedColormap object from matplotlib itself. | |
| via that string or as a Colormap object from matplotlib itself. |
There was a problem hiding this comment.
The object is called a ListedColormap, but I will change it.
hydromt/raster.py
Outdated
| bounds_4326_clip = list(obj.raster.transform_bounds("EPSG:4326")) | ||
| bounds_4326_clip[:2] = map( | ||
| max, | ||
| zip(bounds_4326_clip[:2], (-180, -y_ext)), | ||
| ) | ||
| bounds_4326_clip[2:] = map( | ||
| min, | ||
| zip(bounds_4326_clip[2:], (180, y_ext)), | ||
| ) | ||
| obj_clipped_to_pseudo = obj.raster.clip_bbox( | ||
| bounds_4326_clip, | ||
| crs="EPSG:4326", | ||
| ) |
There was a problem hiding this comment.
Wouldn't this do the same? Or am I missing something?
| bounds_4326_clip = list(obj.raster.transform_bounds("EPSG:4326")) | |
| bounds_4326_clip[:2] = map( | |
| max, | |
| zip(bounds_4326_clip[:2], (-180, -y_ext)), | |
| ) | |
| bounds_4326_clip[2:] = map( | |
| min, | |
| zip(bounds_4326_clip[2:], (180, y_ext)), | |
| ) | |
| obj_clipped_to_pseudo = obj.raster.clip_bbox( | |
| bounds_4326_clip, | |
| crs="EPSG:4326", | |
| ) | |
| obj_clipped_to_pseudo = obj.raster.clip_bbox( | |
| [-180, -y_ext, 180, y_ext], | |
| crs="EPSG:4326", | |
| ) |
There was a problem hiding this comment.
No, I just wrote it way too explicitly..., kind of laughed when I saw this.
|
Done all the last stuff, so I'll merge it. |
Issue addressed
Fixes #345
Explanation
Created tiling function by starting from the bottom in terms of zoomlevel. This starting level is user defined or defaulted to 5.
Checklist
mainAdditional Notes (optional)
Add any additional notes or information that may be helpful.