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

Remove rasterio dep on pyproj #1182

Merged
merged 4 commits into from
May 30, 2023
Merged

Remove rasterio dep on pyproj #1182

merged 4 commits into from
May 30, 2023

Conversation

banesullivan
Copy link
Contributor

Draft work to remove the rasterio source module's dependence on pyproj.

Considering how easy it is to install pyproj these days, I don't think this is totally necessary but it's probably best to have the rasterio source module use rasterio directly for everything rather than pulling an additional dependency.

There is still one remaining function getPixelSizeInMeters() that I wasn't immediately sure on how to reproduce with rasterio

@manthey
Copy link
Member

manthey commented May 30, 2023

We could make a dependency on geographiclib, which is an MIT-licensed pure python library. Then

geod = geographiclib.geodesic.Geodesic.WGS84
s1 = geod.Inverse((bounds['ul']['x'], bounds['ul']['y'], bounds['ur']['x'], bounds['ur']['y'])['s12']
...

Really, getBounds is doing the heavy lifting here.

@banesullivan
Copy link
Contributor Author

I think geographiclib is C++ with Python wrappings? Either way, I think the goal with this PR should be to reduce dependencies for this operation

@manthey
Copy link
Member

manthey commented May 30, 2023

I think geographiclib is C++ with Python wrappings? Either way, I think the goal with this PR should be to reduce dependencies for this operation

The geographiclib python package seems to be pure python without any lib dependencies, but reducing dependencies is worthwhile in and of itself. We are using pyproj to compute the distance between the corner points and using that to compute the average dimension of a pixel. We could have a simple fall back where if pyproj isn't available, we calculate the great circle distance on a sphere and accept that it could have 0.5% error.

@banesullivan
Copy link
Contributor Author

banesullivan commented May 30, 2023

It's worth trying this to at least see how different the results can be for our testing data. @manthey, is that math something you are familiar with or should I do some research into how to do this?

@banesullivan
Copy link
Contributor Author

@12rambau, do you have any thoughts on depending on pyproj with the rasterio source module?

@manthey
Copy link
Member

manthey commented May 30, 2023

It's worth trying this to at least see how different the results can be for our testing data. @manthey, is that math something you are familiar with or should I do some research into how to do this?

We put the Vincenty functions directly into geojs, though we probably should have just pulled in a package. They aren't hard, but fussy and not the sort of thing I think we should have since so many other packages implement that. The simple great circle distance on a sphere is just trig (see https://en.wikipedia.org/wiki/Great-circle_distance) to get an angle multiplied by the average radius of the Earth; if we are doing a fallback to avoid a package, that is probably all we want.

@banesullivan
Copy link
Contributor Author

Sounds good and simple enough! Will add to my todo list to do the great circle estimation then see if it skews results too differently

@banesullivan
Copy link
Contributor Author

That was easy! Tests pass for me locally and as far as I can tell (few surface level tests), there isn't any visible impact on tiles or thumbnails

@banesullivan banesullivan marked this pull request as ready for review May 30, 2023 13:51
@manthey
Copy link
Member

manthey commented May 30, 2023

The biggest difference I see is in sample file test/test_files/rgba_geotiff.tiff, which reports a pixel size of 8.181m rather than 8.170 m.

@banesullivan banesullivan merged commit 7ff54e0 into master May 30, 2023
@banesullivan banesullivan deleted the rasterio-crs branch May 30, 2023 14:54
@12rambau
Copy link
Contributor

oups I'm a bit late to the party. I'm usually depending on the CRS module of rasterio which is sufficient for my applications. It seems a bit more complex from your side but the solution you found is elegant enough.

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.

3 participants