GeoDataFrame init

[AndreiKingsley]

No description provided.

[Jolanrensen]

good initial version :) just some comments here and there

[Jolanrensen]

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

[zaleslaw]

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

1. It's stable for enough time
2. We not guarantee for a while all our releases of experimental geo-module

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

1. Checking the API availability and handling the unavailability with some exception
2. Checking the API consistency and notification for the users or raise an exception
3. Customize it with a URL parameter (if this API just will be moved in another place)

[AndreiKingsley]

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.

[Jolanrensen]

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.

[zaleslaw]

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

[zaleslaw]

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)

[AndreiKingsley]

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

[Jolanrensen]

please check whether you can do that before this is merged

[AndreiKingsley]

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]

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

1. It's stable for enough time
2. We not guarantee for a while all our releases of experimental geo-module

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

1. Checking the API availability and handling the unavailability with some exception
2. Checking the API consistency and notification for the users or raise an exception
3. Customize it with a URL parameter (if this API just will be moved in another place)

[zaleslaw]

Could be ENUM of different constants

[AndreiKingsley]

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.

[zaleslaw]

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

[zaleslaw]

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

[AndreiKingsley]

Done

[Jolanrensen]

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

[Jolanrensen]

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

[AndreiKingsley]

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

[Jolanrensen]

do you mean the gradle plugin or the compiler plugin?

The compiler plugin is highly experimental still and it doesn't work yet in notebooks. So I wouldn't rely on that for now.

The gradle plugin, however, could generate these automatically based on the @DataSchema interfaces you defined. You'd have to add the dataframe gradle plugin to the geo module, just like we did in core, and make sure the generated files are part of sources to they are published along with the rest of the code.

[koperagen]

Sources generated by the plugin are automatically included. Makes sense to double check, but it's expected that you don't need to do anything extra. Just add this and it will generate required properties for annotated interfaces
plugins {
id("org.jetbrains.kotlinx.dataframe")
}

[AndreiKingsley]

Oops, forgot to add it, thank you!

[Jolanrensen]

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]

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

[zaleslaw]

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

[zaleslaw]

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

[Jolanrensen]

Great job :D Only thing missing is a README.md in the module folder like:

## :dataframe-geo

This module, published as `dataframe-geo`, contains all logic and tests for DataFrame to be able to work with geographical data... etc.

So people looking through the codebase know what the folder is for. You also don't have the binary.compatibility.validator gradle plugin yet, but I think that can wait for now, as it's still experimental. Otherwise I approve