-
Notifications
You must be signed in to change notification settings - Fork 30
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
Geometry bounds are updated with the projection. #144
Conversation
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.
Could you add some integration tests that make this fail before the changes?
@naschmitz Test cases for the above have been added. |
I think that if the user don't provide the projection the code will break. I'm not sure, but I think I noted it when I was implementing a very similar change in a previous PR: #127 |
@lbferreira If user not passed any projection code still works but returns the wrong output which current code returns. Code:
|
I don't think we should make any changes based on this case.
|
Hello, @jdbcode. I have encountered an additional practical example in which we required the use of
When rotating the geometry by 180 degrees, the output data should also be rotated by 180 degrees. However, the current XEE code does not rotate the data, while the code from the PR does rotate the data and passes the above test case. |
Geom constructors use EPSG:4326 as default CRS, which has longitude range of -180 to 180 with the prime meridian at 0. You are specifying geometry whose east limit is the prime meridan and wraps around the globe fully, crossing the antimeridian before meeting itself back at the prime meridian. Crossing the antimeridian is likely to give unexpected results. If you are trying to extract data in such a way that the center is at -180 (the antimeridian) you may need to download the data into two parts (east of the antimeridian and west of the antimeridian) and put them together client-side (see antimeridian cutting in the GeoJSON docs), or use a CRS whose center is the antimeridian (maybe something like EPSG:3832?). |
I still worry that it will introduce coordinate precision issues that might affect some people. But if it fixes your case, that's great, and if it causes issues for other people then we'll have confirmed cases to help us figure out a better solution. |
In the current implementation, we are utilizing
geometry.bounds().getInfo()
to retrieve the bound points. However, this approach yields inconsistent results when thelatitude value
of a point exceeds104
, resulting in the return of aconstant latitude value
. So I am adding the projection into the bound points to fix this issue.Example:
The above ds returns only a
single latitude point
when utilizing the current code. However, upon updating the bounds withbounds(proj = projection)
, the ds accurately returns 623 latitude points.Thank you @bbabenko for suggesting the above apporoach.