Refactor/web optimized arg #279
Conversation
rio_cogeo/scripts/cli.py
Outdated
| gdal_tif_ovr_blocksize = blocksize | ||
|
|
||
| if gdal_tif_ovr_blocksize: | ||
| config.update({"GDAL_TIFF_OVR_BLOCKSIZE": gdal_tif_ovr_blocksize}) |
There was a problem hiding this comment.
I tried to keep the previous behaviour, but instead of relying on web-optimization for it, I changed it to be determined by user specified blocksize(s). But this one is part of the problem I describe in the last bullet point of the PR description.
But the main problem I have here is that if I understand GDAL documentation, it should match tile matrix set specification. But with the current way of processing the command argument, I think this is not the case.
I tried to set it as well as possible to match current test cases, but I think that there is something unclear there.
Note that if you prefer, I can change back the code as it was (i.e. force overview blocksize to input blocksize).
In such case, maybe I can raise a different issue focused on this subject, and keep the current PR only focused on web_optimized in python API.
There was a problem hiding this comment.
agreed!
we should set the block size based on https://github.com/developmentseed/morecantile/blob/main/morecantile/models.py#L282-L297 I think for now we should try to match what was in place and we can change it in another PR
There was a problem hiding this comment.
FYI: I remember why we had this
by default we have the overview block size set at 128 or GDAL_TIFF_OVR_BLOCKSIZE. When we were using the web-optimized option we basically wanted the overview block size to be the same as the high resolution blocksize (hence why we have overview_blocksize = blocksize or 512).
The default profiles use 512 block size which explain why we have blocksize or 512.
Next
This is a bit tricky but when we want to web-optimized a file we want all the blocksize to be the same. Right now we default to the profile block size but we should default to the TMS blocksize
There was a problem hiding this comment.
From my understanding, there might be two actions that could clarify/improve configuration:
- Some cli parameters conflict with each other. For example, if user set
--overview-blocksize=256,--blocksize=512and--web-optimized, cogeo receives conflicting configuration. I think that some validation and warnings could be emitted in such cases. - Identified conflicting parameters involve target tile matrix set definition. One way to detect such conflict early and simplify configuration would be to always:
a. build a TileMatrixSet from user input parameters (for example, if user only setblocksize, a TileMatrixSet with the same CRS and bbox as input dataset could be built).
b. Then, and only then, prepare copy parameters from the tile matrix set.
That would surely involve more work internally, but it would cleanly split concerns: user input validation/interpretation one one side, and gdal copy parameterization on the other side. And the bridge between them would be the TileMatrixSet object (at least for tiling and level of details).
Plus, we would then always have a TileMatrixSet specification as intermediate representation, which, I think, would ease debug and configuration sharing.
There was a problem hiding this comment.
Some cli parameters conflict with each other. For example, if user set --overview-blocksize=256, --blocksize=512 and --web-optimized, cogeo receives conflicting configuration. I think that some validation and warnings could be emitted in such cases.
well, I can see how it's a bit confusing but we need both options, we could totally add a warning saying that overview-blocksize will be ignored when using web-optimized or tilematrixset
a. build a TileMatrixSet from user input parameters (for example, if user only set blocksize, a TileMatrixSet with the same CRS and bbox as input dataset could be built).
This will make things a bit more complex IMO
alexismanin
force-pushed
the
refactor/web_optimized_arg
branch
from
3c3ffc4 to
ccc3f31
Compare
…translate` Previously, the function activated `tms` parameter only when set with `web_optimized`. The behaviour has been changed to use `tms` in any case.
Previous command worked only if user executed it from `docs` directory.
alexismanin
force-pushed
the
refactor/web_optimized_arg
branch
from
ccc3f31 to
d6030af
Compare
|
@alexismanin I have reworked my changes to include your suggestions. I also rebased my branch over up-to date main branch. |
| web_profile, | ||
| quiet=True, | ||
| web_optimized=True, | ||
| ) |
Try to prepare for removal of `web_optimized` argument from Python API
alexismanin
force-pushed
the
refactor/web_optimized_arg
branch
from
d6030af to
79ec807
Compare
Changes discussed in #277
Deprecate parameter "web_optimized" of
cogeo.cog_translatePython function.Notes: I would like guidelines about:
web-optimizedoption there, because user cannot pass a tms id (like "WebMercatorQuad"), so I supposeweb-optimizedwill remain prominent there.overview-blocksizeis silently ignored in casetmsorweb-optimizedis specified. I have let it as is, but maybe there is something to change here (echo a warning to the user ? change some logic ?)