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

change how blocksize is defined in create CLI #283

Merged
merged 2 commits into from
Feb 16, 2024

Conversation

vincentsarago
Copy link
Member

ref #279 (review)

closes #280

@vincentsarago
Copy link
Member Author

@alexismanin let me know if you can 👀 this PR 🙏

"GDAL_TIFF_OVR_BLOCKSIZE env or 128)",
show_default=True,
help="Overview's internal tile size (default can be defined by "
"GDAL_TIFF_OVR_BLOCKSIZE env). The default is 128, or starting with GDAL 3.1 to use the same block size as the full-resolution dataset if possible).",
Copy link
Member Author

Choose a reason for hiding this comment

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

remove default in the CLI

tilematrixset.matrix(tilematrixset.minzoom).tileHeight,
tilematrixset.matrix(tilematrixset.minzoom).tileWidth,
)
overview_blocksize = overview_blocksize or blocksize
Copy link
Member Author

Choose a reason for hiding this comment

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

  1. use TMS tileHeight or tileWidth if blocksize is not provided
  2. overview blocksize default to blocksize if not provided (only when there is a tilematrixset option)

Copy link
Contributor

Choose a reason for hiding this comment

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

tilematrix.matrix(tilematrixset.minzoom) gives the lowest-resolution matrix, whereas tilematrix.matrix(tilematrixset.minzoom) returns the best level of details. Therefore, I would tend to define blocksize using tilematrix.matrix(tilematrixset.maxzoom), and keep the minzoom one for overview block size.

Now, I am not sure it matters in practice, because I have not seen any tile-matrix set changing tile size between levels (and in such case, looking at only the min and max level would not be enough anyway).

Copy link
Member Author

Choose a reason for hiding this comment

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

most TMS should have the same tileHeight and tileWidth at all level (at least for the common grids), ideally we would select the blocksize for the zoom level the data cover but we can't get this in the CLI

raster_path_gcps,
],
)
def test_cogeo_info(src_path, runner):
Copy link
Member Author

Choose a reason for hiding this comment

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

not related to this PR main goal

@alexismanin
Copy link
Contributor

@alexismanin let me know if you can 👀 this PR 🙏

I will give a quick look at it.

click.secho(
f"Defining `blocksize` from {tilematrixset.id} TileMatrixSet `tileWidth` and `tileHeight`",
fg="yellow",
)
Copy link
Member Author

Choose a reason for hiding this comment

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

we only print a warning if blocksize wasn't defined

f"Defining overview's `blocksize` to match the high resolution `blocksize`: {blocksize}",
fg="yellow",
)
overview_blocksize = blocksize
Copy link
Member Author

Choose a reason for hiding this comment

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

same here

@vincentsarago vincentsarago merged commit 65b1ab9 into main Feb 16, 2024
5 checks passed
@vincentsarago vincentsarago deleted the features/refactor-blocksize-definition-in-cli branch February 16, 2024 14:01
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.

set blocksize from the TileMatrixSet
2 participants