GeoDataFrame init #909

AndreiKingsley · 2024-10-04T09:15:38Z

No description provided.

Jolanrensen

good initial version :) just some comments here and there

dataframe-geo/build.gradle.kts

Jolanrensen · 2024-10-08T18:24:02Z

dataframe-geo/src/main/kotlin/org/jetbrains/kotlinx/dataframe/geo/GeoDataFrame.kt

+import org.jetbrains.kotlinx.dataframe.api.with
+
+class GeoDataFrame<T : GeoFrame>(val df: DataFrame<T>, val crs: CoordinateReferenceSystem?) {
+    fun update(updateBlock: DataFrame<T>.() -> DataFrame<T>): GeoDataFrame<T> {


Did you consider other names? Like

updateDf

replaceDf (more consistent with what replace does in DF compared to update)

replace implies that we have df1 and df2, and we change df1 to df2. But in reality we are only modifying df1 slightly, using it as a receiver.

Btw, what do you think about modifyDf then?

@koperagen @zaleslaw

In my opinion "Df" is redundant and doesn't look good. Plus it's already clear from signature that it applies to DataFrame

maybe .modify {} or .transform {} are okay?

transform doesn't look good to me.

@koperagen omitting "df" makes function not so obvious that it's the dataframe update and not something else.

dataframe-geo/src/main/kotlin/org/jetbrains/kotlinx/dataframe/geo/GeoDataFrame.kt

dataframe-geo/build.gradle.kts

Jolanrensen · 2024-10-08T18:49:44Z

dataframe-geo/src/main/kotlin/org/jetbrains/kotlinx/dataframe/geo/geocode/Geocoder.kt

+
+object Geocoder {
+
+    private val url = "https://geo2.datalore.jetbrains.com/map_data/geocoding"


Should this be included in the stock library? As far as I remember this was a reverse engineering endeavour and since it depends on an external URL behaviour might change or stop working entirely

For this experimental module, I am totally fine with including in library.

It's stable for enough time

We not guarantee for a while all our releases of experimental geo-module

But we could also include this solution for the @Jolanrensen question:

Checking the API availability and handling the unavailability with some exception

Checking the API consistency and notification for the users or raise an exception

Customize it with a URL parameter (if this API just will be moved in another place)

Geocoder is not part of the main API and will be removed/reworked completely (via future Kotlin geocoding library we discussed). I kept it, exclusively to play with it. So no effort should be wasted on it.

Probably fine then :) You could add a small note in KDocs too telling this is experimental/temporary, so potential users won't get used to it.

settings.gradle.kts

dataframe-geo/src/main/kotlin/org/jetbrains/kotlinx/dataframe/jupyter/IntegrationGeo.kt

dataframe-geo/src/main/kotlin/org/jetbrains/kotlinx/dataframe/geo/io/read.kt

...eo/src/main/kotlin/org/jetbrains/kotlinx/dataframe/geo/geotools/toSimpleFeatureCollection.kt

dataframe-geo/src/main/kotlin/org/jetbrains/kotlinx/dataframe/geo/geotools/ToGeoDataFrame.kt

zaleslaw

Generally, I am approving these changes, but want to summarize some thoughts and make this piece of code more mature and maintanable

zaleslaw · 2024-11-04T15:49:42Z

dataframe-geo/build.gradle.kts

+repositories {
+    // geo repositories should come before Maven Central
+    maven("https://maven.geotoolkit.org")
+    maven("https://repo.osgeo.org/repository/release")


Is it true, that these maven repo links will be hidden in the module, and the final user don't need to refer to them somehow in its Gradle script? (just for self checking)

Unfortunately, the user will need to add the repository. However, I believe we can fix this with shadow jar.

please check whether you can do that before this is merged

I'd leave it that way for now, then I'll try to get rid of it. When using a descriptor in notebook, the user won't need to do this.

zaleslaw · 2024-11-04T15:50:13Z

dataframe-geo/build.gradle.kts

+    useJUnitPlatform()
+}
+kotlin {
+    jvmToolchain(11)


Could we downgrade it somehow to 8?

We have a global problem - DataFrame uses 8 and the Jupyter API uses 11. Therefore, in the future it will be necessary to allocate separate module(s) for integrations.

but for now, for convenience, I'd keep it 11

Actually, you can probably remove this piece, as kotlin.jvmToolChain(11) is already called from the main build.gradle.kts file; It configures all subprojects automatically to target jvm 8 in the published jars while setting the jvm toolchain (so the version gradle uses to build and test the project) to 11.

dataframe-geo/src/main/kotlin/org/jetbrains/kotlinx/dataframe/jupyter/IntegrationGeo.kt

zaleslaw · 2024-11-04T16:06:00Z

dataframe-geo/src/main/kotlin/org/jetbrains/kotlinx/dataframe/geo/GeoFrame.kt

+import org.locationtech.jts.geom.Polygon
+
+@DataSchema
+interface GeoFrame {


The name is lead in the wrong direction. It's not GeoFrame. It's close to wrappers for Geometry or Form or GeometryForm

I agree, that geometry is produced from Geo, but it's misleading me

Because we have Geometry from jts, what do you think about Form or Figure or GeometryFigure/GeometryType or GeometryColumnType, verbose but clear

WithGeometry?

zaleslaw · 2024-11-04T16:11:18Z

dataframe-geo/src/main/kotlin/org/jetbrains/kotlinx/dataframe/geo/geocode/Geocoder.kt

+
+object Geocoder {
+
+    private val url = "https://geo2.datalore.jetbrains.com/map_data/geocoding"


For this experimental module, I am totally fine with including in library.

It's stable for enough time

We not guarantee for a while all our releases of experimental geo-module

But we could also include this solution for the @Jolanrensen question:

Checking the API availability and handling the unavailability with some exception

Checking the API consistency and notification for the users or raise an exception

Customize it with a URL parameter (if this API just will be moved in another place)

dataframe-geo/src/main/kotlin/org/jetbrains/kotlinx/dataframe/geo/GeoDataFrame.kt

dataframe-geo/src/main/kotlin/org/jetbrains/kotlinx/dataframe/geo/jts/geometryExtenstions.kt

zaleslaw · 2024-11-04T16:57:29Z

dataframe-geo/src/main/kotlin/org/jetbrains/kotlinx/dataframe/geo/jts/geometryExtenstions.kt

+    return transformation.transform(this)
+}
+
+fun Geometry.translate(valueX: Double, valueY: Double): Geometry {


Could you consider renaming to somthing like shift/move for simplicity

I think "translate" is a common name in the math/geometry-world

the main issue, that we are doing this not for math guys, but for Kotlin developers who are not familiar with all these concepts, and I am trying to make the naming simpler and more obvious

it's a well-established concept, so I can't agree:
https://en.wikipedia.org/wiki/Translation_(geometry)

I agree with Andrei on this one. "translate" is common enough I think

zaleslaw · 2024-11-04T16:58:51Z

dataframe-geo/src/main/kotlin/org/jetbrains/kotlinx/dataframe/geo/GeoDataFrame.kt

+    }
+
+    companion object {
+        val DEFAULT_CRS = CRS.decode("EPSG:4326", true)


Could be ENUM of different constants

Yes, it even exists in some form in GeoTools (see DefaultEngineeringCRS). But here is a more global question - whether it is necessary to use geotools API directly or should we wrap it completely.

I suggest not wrap for now, we could not avoid the learning of that

zaleslaw · 2024-11-04T17:00:50Z

dataframe-geo/src/main/kotlin/org/jetbrains/kotlinx/dataframe/geo/io/write.kt

+
+fun GeoDataFrame<*>.writeGeoJson(path: String): Unit = writeGeoJson(File(path))
+
+fun GeoDataFrame<*>.writeGeoJson(file: File) {


Please add for IO part some JUnit tests for reading/writing files close to your examples https://gist.github.com/AndreiKingsley/8eada06f174c681e953f8d939248d153

dataframe-geo/src/main/kotlin/org/jetbrains/kotlinx/dataframe/geo/geocode/Geocoder.kt

Jolanrensen · 2024-11-06T12:13:33Z

dataframe-geo/src/main/kotlin/org/jetbrains/kotlinx/dataframe/geo/jts/geometryExtenstions.kt

@@ -0,0 +1,79 @@
+package org.jetbrains.kotlinx.dataframe.geo.jts


typo in filename: *Extensions

Jolanrensen · 2024-11-06T12:14:49Z

dataframe-geo/src/main/kotlin/org/jetbrains/kotlinx/dataframe/geo/io/read.kt

+
+
+fun GeoDataFrame.Companion.readGeoJson(path: String): GeoDataFrame<*> {
+    return readGeoJson(asURL(path))


One thing the linter will complain about is that these should be expression functions. One ktLintFormat can replace them all at once tho

Jolanrensen · 2024-11-06T12:25:59Z

dataframe-geo/src/main/kotlin/org/jetbrains/kotlinx/dataframe/geo/GeoDataFrame.kt

+import org.jetbrains.kotlinx.dataframe.api.update
+import org.jetbrains.kotlinx.dataframe.api.with
+
+/**


good docs :D

I gave you a compliment haha

Jolanrensen · 2024-11-06T12:27:21Z

dataframe-geo/src/main/kotlin/org/jetbrains/kotlinx/dataframe/geo/GeoDataFrame.kt

+ */
+class GeoDataFrame<T : WithGeometry>(val df: DataFrame<T>, val crs: CoordinateReferenceSystem?) {
+    /**
+     * Updates the `GeoDataFrame` using a specified transformation block on the underlying DataFrame.


Only thing I'd mention in these functions is that it doesn't simply "update" (which would mean the class is mutable), but it returns a new GeoDataFrame copy with the dataframe being replaced

Jolanrensen · 2024-11-06T12:28:02Z

dataframe-geo/src/main/kotlin/org/jetbrains/kotlinx/dataframe/geo/GeoDataFrame.kt

+        )
+    }
+
+    override fun equals(other: Any?): Boolean {


missing hashCode and toString functions

Jolanrensen · 2024-11-06T12:35:32Z

dataframe-geo/src/main/kotlin/org/jetbrains/kotlinx/dataframe/geo/WithGeometry.kt

+}
+
+@get:JvmName("geometry")
+val <T : WithGeometry> ColumnsContainer<T>.geometry: DataColumn<Geometry>


I think the T is not needed, right? ColumnsContainer<WithGeometry> has the same effect

Also, for the generated accessor we have both ColumnsContainer and DataRow extensions, so probably you should add DataRow<WithGeometry>.geometry: Geometry etc. too :)

@koperagen could plugin help here, to generate all these extensions?

Jolanrensen · 2024-11-06T12:58:32Z

Left a couple more comments :) Thanks for fixing some stuff already!
Also, don't forget to update from master. Most important recent changes include:

a README.md file in each module with a small description of what the module does
binary compatibility checker, you can add the plugin, run apiDump and add the api file to git just before merging this PR

zaleslaw · 2024-11-06T13:32:01Z

dataframe-geo/src/main/kotlin/org/jetbrains/kotlinx/dataframe/geo/geotools/toGeoDataFrame.kt

+import org.jetbrains.kotlinx.dataframe.geo.WithGeometry
+import org.locationtech.jts.geom.Geometry
+
+fun SimpleFeatureCollection.toGeoDataFrame(): GeoDataFrame<*> {


Please document it with KDocs, looks like it will not change too much in future

It's worth to add in comment the summarization of the underlying algorithm

zaleslaw · 2024-11-06T13:33:17Z

...eo/src/main/kotlin/org/jetbrains/kotlinx/dataframe/geo/geotools/toSimpleFeatureCollection.kt

+import org.jetbrains.kotlinx.dataframe.geo.GeoDataFrame
+import org.locationtech.jts.geom.Geometry
+
+fun GeoDataFrame<*>.toSimpleFeatureCollection(


Please add a KDoc, with tiny explanation (especially if some edge cases are not working for now)

zaleslaw

For me it looks much more mature today, please fix comments/linter and it could be merged, I believe

GeoDataFrame init

f0967bb

AndreiKingsley requested review from zaleslaw, koperagen and Jolanrensen October 4, 2024 09:15

AndreiKingsley added 2 commits October 4, 2024 13:32

GeoDataFrame add publication

3458ac5

Merge branch 'master' into geo

4bd8105

Jolanrensen requested changes Oct 9, 2024

View reviewed changes

devcrocod mentioned this pull request Oct 11, 2024

Support GeoDataFrame plotting. Kotlin/kandy#441

Open

3 tasks

zaleslaw requested changes Nov 4, 2024

View reviewed changes

AndreiKingsley added 2 commits November 6, 2024 02:02

PR changes & docs

b724f9d

fix crs equals checks

0ff0ee8

AndreiKingsley requested review from zaleslaw and Jolanrensen November 5, 2024 22:11

Kotlin deleted a comment from zaleslaw Nov 6, 2024

Jolanrensen reviewed Nov 6, 2024

View reviewed changes

dataframe-geo/src/main/kotlin/org/jetbrains/kotlinx/dataframe/geo/geocode/Geocoder.kt Outdated Show resolved Hide resolved

Jolanrensen reviewed Nov 6, 2024

View reviewed changes

zaleslaw reviewed Nov 6, 2024

View reviewed changes

zaleslaw approved these changes Nov 6, 2024

View reviewed changes

docs & format

289a1fb


		object Geocoder {

		private val url = "https://geo2.datalore.jetbrains.com/map_data/geocoding"


		fun GeoDataFrame<*>.writeGeoJson(path: String): Unit = writeGeoJson(File(path))

		fun GeoDataFrame<*>.writeGeoJson(file: File) {

		@@ -0,0 +1,79 @@
		package org.jetbrains.kotlinx.dataframe.geo.jts



		fun GeoDataFrame.Companion.readGeoJson(path: String): GeoDataFrame<*> {
		return readGeoJson(asURL(path))

GeoDataFrame init #909

Are you sure you want to change the base?

GeoDataFrame init #909

Conversation

AndreiKingsley commented Oct 4, 2024

Jolanrensen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndreiKingsley Nov 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zaleslaw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndreiKingsley Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zaleslaw Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jolanrensen commented Nov 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zaleslaw left a comment

Choose a reason for hiding this comment

AndreiKingsley Nov 6, 2024 •

edited

Loading

AndreiKingsley Nov 5, 2024 •

edited

Loading

zaleslaw Nov 4, 2024 •

edited

Loading