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

[SEDONA-346] Add WorldToRaster APIs #948

Merged
merged 36 commits into from
Aug 9, 2023

Conversation

iGN5117
Copy link
Contributor

@iGN5117 iGN5117 commented Aug 8, 2023

Did you read the Contributor Guide?

Is this PR related to a JIRA ticket?

What changes were proposed in this PR?

  • Add RS_WorldToRasterCoord, RS_WorldToRasterCoordX, RS_WorldToRasterCoordY

How was this patch tested?

  • Added new tests

Did this PR include necessary documentation updates?

# Conflicts:
#	common/src/main/java/org/apache/sedona/common/raster/RasterAccessors.java
#	docs/api/sql/Raster-operators.md
#	sql/common/src/main/scala/org/apache/sedona/sql/UDF/Catalog.scala
#	sql/common/src/main/scala/org/apache/spark/sql/sedona_sql/expressions/raster/RasterAccessors.scala
# Conflicts:
#	sql/common/src/main/scala/org/apache/sedona/sql/UDF/Catalog.scala
#	sql/common/src/test/scala/org/apache/sedona/sql/rasteralgebraTest.scala
Impose assert on returned point's SRID
Refactor thrown exception to be IndexOutOfBounds
# Conflicts:
#	docs/api/sql/Raster-operators.md
#	sql/common/src/main/scala/org/apache/sedona/sql/UDF/Catalog.scala
#	sql/common/src/test/scala/org/apache/sedona/sql/rasteralgebraTest.scala
# Conflicts:
#	common/src/main/java/org/apache/sedona/common/raster/PixelFunctions.java
#	common/src/main/java/org/apache/sedona/common/raster/RasterAccessors.java
#	common/src/main/java/org/apache/sedona/common/utils/RasterUtils.java
#	common/src/test/java/org/apache/sedona/common/raster/RasterAccessorsTest.java
#	sql/common/src/main/scala/org/apache/spark/sql/sedona_sql/expressions/raster/RasterAccessors.scala
#	sql/common/src/test/scala/org/apache/sedona/sql/rasteralgebraTest.scala
import org.opengis.referencing.operation.MathTransform;
import org.opengis.referencing.operation.TransformException;

import static org.apache.sedona.common.utils.RasterUtils.convertCRSIfNeeded;
Copy link
Member

Choose a reason for hiding this comment

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

This import is not correct. Please fix this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactored to import RasterUtils instead

@@ -95,7 +100,7 @@ public static AffineTransform2D getAffineTransform(GridCoverage2D raster, PixelO
return (AffineTransform2D) crsTransform;
}

public static Point2D getCornerCoordinates(GridCoverage2D raster, int colX, int rowY) throws TransformException {
public static Point2D getWorldCornerCoordinates(GridCoverage2D raster, int colX, int rowY) throws TransformException {
Copy link
Member

Choose a reason for hiding this comment

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

The problem might not be related to this PR. However, I think only getWorldCornerCoordinatesWithRangeCheck should be kept. I understand some PostGIS flavors do not perform range check. Do we actually support the computation of out of boundary raster coordinate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes we actually do support out of raster boundary coordinates as well. GridCoverage2D transform does not do any range checking on it own and hence its easy for us to support going out of bounds assuming the same skew and translate

return RasterUtils.getWorldCornerCoordinates(raster, colX, rowY).getY();
}

public static Geometry getGridCoord(GridCoverage2D raster, double longitude, double latitude) throws TransformException {
Copy link
Member

Choose a reason for hiding this comment

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

Given that the CRS transformation follows the lat/lon order, the parameters here are wrong. To avoid unnecessary complication, please call them X/Y, instead of longitude and latitude.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, changed lon and lat to x and y respectively

@jiayuasu jiayuasu added this to the sedona-1.5.0 milestone Aug 9, 2023
@jiayuasu jiayuasu merged commit 063c4f6 into apache:master Aug 9, 2023
39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants