-
Notifications
You must be signed in to change notification settings - Fork 654
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
[SEDONA-425] Add RS_Values and RS_Value to accept grid coordinates #1122
Conversation
Refactor examples folder to include correct updated Sedona version
…_master # Conflicts: # binder/ApacheSedonaSQL.ipynb # binder/ApacheSedonaSQL_SpatialJoin_AirportsPerCountry.ipynb
Add inferrable support for List<Double> and List<Geometry>
double x = xCoordinates[i]; | ||
double y = yCoordinates[i]; | ||
|
||
GridCoordinates2D gridCoord = new GridCoordinates2D((int) x, (int) y); |
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.
Is there a reason to convert the int coordinate to double and then back to int?
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.
Fixed. That was a remanent of my previous approach. Thanks.
|
||
Format: | ||
|
||
`RS_Value (raster: Raster, point: Geometry)` | ||
|
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.
Please keep RS_Value (raster: Raster, point: Geometry)
signature because we don't want to introduce any API breaking change.
Then revert the change R test because it is no longer needed.
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.
Have kept RS_Value (raster: Raster, point: Geometry)
signature and reverted changes to R tests.
Note:
- Only Point geometry variant of RS_Value and RS_Values can take band as optional parameter
- Same currently not possible for the integer variant because of conflicting function signatures as shown below,
RS_Value (raster: Raster, colX: Integer, colY: Integer)
RS_Value (raster: Raster, point: Geometry, band: Integer)
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.
Can you also add the explanation to the RS_Value and RS_Values doc: the input geometry point must be in the same CRS of the raster.
This reverts commit 67b2bc2.
|
||
Format: | ||
|
||
`RS_Value (raster: Raster, point: Geometry)` | ||
|
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.
Can you also add the explanation to the RS_Value and RS_Values doc: the input geometry point must be in the same CRS of the raster.
@jiayuasu Added a note in docs to match CRS of raster and Point geometries. |
Did you read the Contributor Guide?
Is this PR related to a JIRA ticket?
What changes were proposed in this PR?
How was this patch tested?
Did this PR include necessary documentation updates?