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

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

Closed
wants to merge 2 commits into from

Conversation

DanSchoppe
Copy link
Contributor

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 Reader `resampling_method` has no effect cogeotiff/rio-tiler#646 (reply in thread)

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.

-e "src/titiler/core[test]" \
-e "src/titiler/extensions[test,cogeo,stac]" \
-e "src/titiler/mosaic[test]" \
-e "src/titiler/application[test]"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this was just some invalid syntax. It tripped me up so I thought I'd sneak this change in to help others.

Copy link
Contributor Author

@DanSchoppe DanSchoppe Oct 17, 2023

Choose a reason for hiding this comment

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

@vincentsarago
Copy link
Member

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
Copy link
Member

@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
Copy link
Contributor Author

@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
Copy link
Member

🤦 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
Copy link
Contributor Author

DanSchoppe commented Oct 19, 2023

@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
Copy link
Member

done in #717

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.

2 participants