Geo #449
Geo #449
Conversation
# Conflicts: # kandy-lets-plot/src/main/kotlin/org/jetbrains/kotlinx/kandy/letsplot/translator/toLetsPlot.kt # kandy-lets-plot/src/main/kotlin/org/jetbrains/kotlinx/kandy/letsplot/translator/util.kt
kandy-lets-plot/src/main/kotlin/org/jetbrains/kotlinx/kandy/letsplot/translator/layerWrap.kt
Show resolved
Hide resolved
kandy-lets-plot/src/main/kotlin/org/jetbrains/kotlinx/kandy/letsplot/translator/layerWrap.kt
Show resolved
Hide resolved
| import org.jetbrains.kotlinx.kandy.ir.data.TableData | ||
| import org.jetbrains.letsPlot.spatial.SpatialDataset | ||
|
|
||
| public interface GeoSpatialData: TableData { |
| import org.locationtech.jts.geom.MultiPolygon | ||
| import org.locationtech.jts.operation.union.CascadedPolygonUnion | ||
|
|
||
|
|
There was a problem hiding this comment.
Remove empty line and add some KDocs, great that exception is raised? Could we throw IllegalArgumentException in other situation which I commented?
| ) { | ||
| geometry().forEach { | ||
| if (it !is Polygonal) { | ||
| error("Not a polygon geometry: $it") |
There was a problem hiding this comment.
why it somewhere IllegalArgumentException, somewhere error? could it be unified through the PR?
| import org.locationtech.jts.geom.Puntal | ||
| import kotlin.reflect.typeOf | ||
|
|
||
| // TODO add ColumnAccessor & String api |
kandy-geo/src/main/kotlin/org/jetbrains/kotlinx/kandy/letsplot/geo/dsl/GeoDataScope.kt
Show resolved
Hide resolved
kandy-geo/src/main/kotlin/org/jetbrains/kotlinx/kandy/letsplot/geo/dsl/acсessors.kt
Outdated
Show resolved
Hide resolved
| import org.jetbrains.kotlinx.kandy.dsl.internal.LayerCreatorScope | ||
| import org.jetbrains.kotlinx.kandy.dsl.internal.MultiLayerPlotBuilder | ||
|
|
||
| public class GeoDataFrameScope<T : WithGeometry>( |
...-geo/src/main/kotlin/org/jetbrains/kotlinx/kandy/letsplot/geo/dsl/GeoDataFramePlotBuilder.kt
Show resolved
Hide resolved
| override fun toSpatialDataset(): SpatialDataset { | ||
| with(geoDataFrame) { | ||
| // TODO encoding precision | ||
| val geojson = GeometryJSON(10) |
There was a problem hiding this comment.
Magic Constant - should be extracted and explained
| with(geoDataFrame) { | ||
| // TODO encoding precision | ||
| val geojson = GeometryJSON(10) | ||
| return SpatialDataset.withGEOJSON( |
There was a problem hiding this comment.
is it possible to rename to withGeoJson to be more Kotlinish?
There was a problem hiding this comment.
It's not our API
| val friendModules = listOf(project(":kandy-api"), project(":kandy-lets-plot")) | ||
| val jarTasks = friendModules.map { it.tasks.getByName("jar") as Jar } | ||
| val jarPaths = jarTasks.map { it.archiveFile.get().asFile.absolutePath } | ||
| (this as BaseKotlinCompile).friendPaths.from(jarPaths) |
There was a problem hiding this comment.
could please add some note or explanation about why do we need friend modules here?
There was a problem hiding this comment.
@AndreiKingsley
add a comment with motivation "what's happened here? and why we did this?"
in this tasks.withType snippet
kandy-geo/build.gradle.kts
Outdated
| // jai core dependency should be excluded from geotools dependencies and added separately | ||
| fun ExternalModuleDependency.excludeJaiCore() = exclude("javax.media", "jai_core") | ||
|
|
||
|
|
zaleslaw
left a comment
zaleslaw
left a comment
There was a problem hiding this comment.
It requires more work on KDocs and some questions should be answered also
| * | ||
| * This interface extends the `TableData` interface, providing specific capabilities for handling geospatial data. | ||
| * | ||
| * @property dataFrame The data frame containing the geospatial data. |
There was a problem hiding this comment.
The DataFrame (to refer on class), don't use data frame
| if (crs == null || crs.isWGS84() == true) { | ||
| (this as LayerCreatorScope).plotBuilder.coordinatesTransformation = CoordinatesTransformation.mercator() | ||
| } else { | ||
| //TODO handling other CRS |
There was a problem hiding this comment.
Please create a ticket and describe a list of "other CRS" - could be an umbrella ticket
| val geometryColumn = geometry named "geometry" | ||
| val geoDataFrame = if (columnRowsCount != datasetBuilderRowsCount) { | ||
| // override dataset if datasetBuilder size doesn't match | ||
| dataFrameOf(geometryColumn).toGeo(null) |
There was a problem hiding this comment.
I don't understand this code dataFrameOf(geometryColumn).toGeo(null)
There was a problem hiding this comment.
Creating a new dataframe with single column and casting to GeoDataFrame (witn null crs)
There was a problem hiding this comment.
This is not quite in Kotlin style. You can define a copy method with changing parameters
There was a problem hiding this comment.
What do you mean?
zaleslaw
left a comment
zaleslaw
left a comment
There was a problem hiding this comment.
Some moments a little bit unclear, please clarify them
devcrocod
left a comment
devcrocod
left a comment
There was a problem hiding this comment.
I left a few comments
Unfortunately, I don't have the opportunity to look more closely later, but overall I approved this
| val rowsCount = group.rowsCount() | ||
| val keyColumns = keyNames.filter { it !in group.columnNames() }.map { keyName -> | ||
| DataColumn.create(keyName, List(rowsCount) { key[keyName] }, Infer.Type) | ||
| DataColumn.Companion.createByType(keyName, List(rowsCount) { key[keyName] }, Infer.Type) |
|
|
||
| companion object { | ||
| // TODO lets-plot default, encoding precision customization | ||
| const val DEFAULT_PRECISION = 10 |
There was a problem hiding this comment.
Is there a way to change this?
Why is it in a companion object and a constant?
There was a problem hiding this comment.
it's temporary, will design better way to specify it
There was a problem hiding this comment.
Will add an issue
| val geometryColumn = geometry named "geometry" | ||
| val geoDataFrame = if (columnRowsCount != datasetBuilderRowsCount) { | ||
| // override dataset if datasetBuilder size doesn't match | ||
| dataFrameOf(geometryColumn).toGeo(null) |
There was a problem hiding this comment.
This is not quite in Kotlin style. You can define a copy method with changing parameters
| block: PolygonBuilder.() -> Unit = {} | ||
| ) { | ||
| geometry().forEach { | ||
| if (it !is Polygonal) { |
| } | ||
|
|
||
| // TODO add ColumnAccessor & String api | ||
| @JvmName("geoRectanglesGeometry") |
There was a problem hiding this comment.
Why do we need JvmName? Is there a conflict somewhere?
There was a problem hiding this comment.
Yes:
Platform declaration clash: The following declarations have the same JVM signature (geoRectangles(Lorg/jetbrains/kotlinx/kandy/dsl/internal/LayerCreatorScope;Ljava/lang/Iterable;Lkotlin/jvm/functions/Function1;)V):
fun LayerCreatorScope.geoRectangles(geometry: Iterable<Envelope>, block: (RectanglesBuilder) -> Unit = ...): Unit defined in org.jetbrains.kotlinx.kandy.letsplot.geo.layers
fun LayerCreatorScope.geoRectangles(geometry: Iterable<Geometry>, block: (RectanglesBuilder) -> Unit = ...): Unit defined in org.jetbrains.kotlinx.kandy.letsplot.geo.layers
No description provided.