developmentseed / titiler

Build your own Raster dynamic map tile services
https://developmentseed.org/titiler/
MIT License
765 stars 157 forks source link

Add a `reproject` query parameter to be passed as a `reproject_method` argument to rio-tiler #714

Closed DanSchoppe closed 10 months ago

DanSchoppe commented 11 months ago

What I am changing

rio-tiler v5 adds support for a new reproject_method argument. This PR adds a reproject query parameter that's passed as a reproject_method argument to rio-tiler, similar to how the resampling query param is passed to rio-tiler as resampling_method.

How I did it

I followed the prior art from resampling and added this in /src/titiler/core/titiler/core/dependencies.py as a DatasetParam with a default value and query param alias.

How you can test it

I tested this in two ways:

  1. I added a test to /src/titiler/core/tests/test_dependencies.py to prove that the reproject query parameter was successfully being passed as a reproject_method argument to rio-tiler. I also test that the value defaults to "nearest" if left unspecified.
  2. I deployed the change to a Lambda-hosted instance of TiTiler and confirmed that reproject had the desired effect on a requested tile. See https://github.com/cogeotiff/rio-tiler/discussions/646#discussioncomment-7305095

Related Issues

NOTE: I believe I need to update the docs, but need some help with this. This command...

pdocs as_markdown --output_dir docs/src/api --exclude_source --overwrite titiler.core.dependencies titiler.core.factory titiler.core.utils titiler.core.routing titiler.core.errors titiler.core.resources.enums titiler.core.middleware

... produces a pretty massive diff:

image

Maybe I need a specific version of pdocs? Sorry, I'm new to pdocs.

vincentsarago commented 11 months ago

I believe I need to update the docs, but need some help with this. This command...

This is done automatically in CI https://github.com/developmentseed/titiler/blob/main/.github/workflows/deploy_mkdocs.yml#L16-L61

you just need to update https://github.com/developmentseed/titiler/blob/main/docs/src/advanced/dependencies.md and the endpoints options https://github.com/developmentseed/titiler/blob/main/docs/src/endpoints/cog.md

vincentsarago commented 11 months ago

@DanSchoppe what do you think about using the resampling option to be used to set both rio-tiler's resampling?

We already have a ton of query-parameters and I feel most user will be fine with only one option (power-user would be able to change the dependency to add the two options)

DanSchoppe commented 11 months ago

@DanSchoppe what do you think about using the resampling option to be used to set both rio-tiler's resampling?

We already have a ton of query-parameters and I feel most user will be fine with only one option (power-user would be able to change the dependency to add the two options)

@vincentsarago Admittedly I don't have a full understanding of the desired interpolation behaviors of downsampling/upsampling/reprojection, but no objection from me if you want me to map TiTiler::resampling query param to both rio-tiler::resampling_method and rio-tiler::reproject_method.

I'm still uneasy about cogeotiff/rio-tiler#646. I'd happily close this PR altogether if we were able to identify there was a regression in rio-tiler's resampling_method behavior.

On the other hand, If we think rio-tiler is acting as desired and my problem stems from the introduction of the new reproject_method argument, I'll update this PR.

vincentsarago commented 11 months ago

🤦 in fact we can't have resampling option to fill for both rio-tiler::resampling_method and rio-tiler::reproject_method because they each have a specific set of values 😢

if we were able to identify there was a regression in rio-tiler's

I don't think there is a regression in rio-tiler but the contrary, it's now more precise IMO, so adding reproject_method is 👍

DanSchoppe commented 11 months ago

@vincentsarago with cogeotiff/rio-tiler#648 fixing the resampling behavior 🥳 , I personally no longer have a desire to specify a reprojection method via TiTiler reproject query param. Feel free to close this if you don't think it provides value to the project, else I'm happy to adjust the PR to see it across the finish line 🏁. Up to you!

vincentsarago commented 10 months ago

done in https://github.com/developmentseed/titiler/pull/717