-
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-280] Add ST_GeometricMedian #831
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.
Thank you for the great contribution!
sql/common/src/main/scala/org/apache/spark/sql/sedona_sql/expressions/st_functions.scala
Show resolved
Hide resolved
Updated docs Added check for geometry type
Added test cases
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.
@akolts Thank you very much for your help! Everything looks good to me except the extra comma :-)
("'MULTIPOINT ((0 0), (10 1), (5 1), (20 20))'", 1e-15) -> "'POINT (5 1)'", | ||
("'MULTIPOINT ((0 -1), (0 0), (0 0), (0 1))'", 1e-6) -> "'POINT (0 0)'", | ||
("'POINT (7 6)'", 1e-6) -> "'POINT (7 6)'", | ||
("'MULTIPOINT ((12 5),(62 7),(100 -1),(100 -5),(10 20),(105 -5))'", 1e-15) -> "'POINT(84.21672412761632 0.1351485929395439)'", |
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.
Scala compilation is failed here: error file=/home/runner/work/sedona/sedona/sql/common/src/test/scala/org/apache/sedona/sql/functionTestScala.scala message=expected start of definition, but was Token(VAL,val,85798,val)
This is likely caused by this comma ,
. Please remove the last comma which is not needed.
Remove coma breaking Scala compilation
Fix data type
Rename parameters
@@ -81,6 +81,7 @@ | |||
(stf.ST_ExteriorRing, ("geom",), "triangle_geom", "", "LINESTRING (0 0, 1 0, 1 1, 0 0)"), | |||
(stf.ST_FlipCoordinates, ("point",), "point_geom", "", "POINT (1 0)"), | |||
(stf.ST_Force_2D, ("point",), "point_geom", "", "POINT (0 1)"), | |||
(stf.ST_GeometricMedian, ("multipoint_geom",), "point_geom", "", "POINT(22.500002656424286 21.250001168173426)"), |
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 believe this line should be
("multipoint",), "multipoint_geom"
Then it will fix the failed Python test case
Fix python test
@@ -81,6 +81,7 @@ | |||
(stf.ST_ExteriorRing, ("geom",), "triangle_geom", "", "LINESTRING (0 0, 1 0, 1 1, 0 0)"), | |||
(stf.ST_FlipCoordinates, ("point",), "point_geom", "", "POINT (1 0)"), | |||
(stf.ST_Force_2D, ("point",), "point_geom", "", "POINT (0 1)"), | |||
(stf.ST_GeometricMedian, ("multipoint",), "multipoint_geom", "", "POINT(22.500002656424286 21.250001168173426)"), |
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.
OK. There should be a space between POINT
and (
. It should be POINT (
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.
@jiayuasu Yeah, I figured. Should have installed python for testing. Thanks for quick reviews and your help.
Fix python test - remove space
What changes were proposed in this PR?
Port of ST_GeometricMean from PostGIS
How was this patch tested?
Test cases and running in internal environment. Checking results against PostGIS.
Did this PR include necessary documentation updates?
Yes