-
Notifications
You must be signed in to change notification settings - Fork 171
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
Conversation
-e "src/titiler/core[test]" \ | ||
-e "src/titiler/extensions[test,cogeo,stac]" \ | ||
-e "src/titiler/mosaic[test]" \ | ||
-e "src/titiler/application[test]" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
How about this? 941acd8
This is done automatically in CI https://github.com/developmentseed/titiler/blob/main/.github/workflows/deploy_mkdocs.yml#L16-L61 you |
@DanSchoppe what do you think about using the 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 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 On the other hand, If we think rio-tiler is acting as desired and my problem stems from the introduction of the new |
🤦 in fact we can't have
I don't think there is a regression in rio-tiler but the contrary, it's now more precise IMO, so adding |
@vincentsarago with cogeotiff/rio-tiler#648 fixing the |
done in #717 |
What I am changing
rio-tiler
v5 adds support for a newreproject_method
argument. This PR adds areproject
query parameter that's passed as areproject_method
argument to rio-tiler, similar to how theresampling
query param is passed to rio-tiler asresampling_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:
/src/titiler/core/tests/test_dependencies.py
to prove that thereproject
query parameter was successfully being passed as areproject_method
argument to rio-tiler. I also test that the value defaults to"nearest"
if left unspecified.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...
... produces a pretty massive diff:
Maybe I need a specific version of
pdocs
? Sorry, I'm new topdocs
.