search

dartclub / turf_dart

A turf.js-like geospatial analysis library working with GeoJSON, written in pure Dart.
https://pub.dev/packages/turf
MIT License
69 stars 31 forks source link

Port Meta Tests and Functions #12

Closed baparham closed 2 years ago

baparham commented 3 years ago

*each

*reduce

other

lukas-h commented 3 years ago

Hi @baparham, any updates?

baparham commented 2 years ago

No 😳 it's been a rather busy year, but I can spend some time on this, I've been wanting to get back into flutter/dart development lately so your ping is timely. I'll block out some time this week to devote here.

lukas-h commented 2 years ago

Nice! I'm also slowly starting to work on other parts of the library again, e.g. null-safety support which is more than overdue

tobrun commented 2 years ago

The most useful meta function to now work on will be coordEach. It's heavily used by other turf functions. Happy to work on that one but might tip my toes in the water with an easy meta function to get started :)

edit: have implementation ready for propEach but depends on merging #13 first

lukas-h commented 2 years ago

When implementing the functions, please let's keep in mind that my GeoJSON implementation follows a very strict class representation of the RFC standard. I would prefer to have stricter types for the functions, e.g.:

- void geomEach(dynamic geoJSON, GeomEachCallback callback) {
+ void geomEach(GeoJSONObject geoJSON, GeomEachCallback callback) {

also, see my comments here: #24 and here: https://github.com/dartclub/turf_dart/issues/14#issuecomment-832898872

here you can see all the classes and their inheritance in a tree diagram: GeoJSON Polymorphism (1)

baparham commented 2 years ago

I agree @lukas-h that these should be typed stricter. Currently the GeoJSONObject doesn't include features, so perhaps that's the first step, is to bring that abstract class up to snuff?

I can try and implement this in a new PR, I'll paste that in here if I get something that works

baparham commented 2 years ago

It seems like I can use some runtime typechecking, and casting with as but I think that means that most of the geomEach function will get rewritten since dart can't handle the way turf.js used the isFeature[Collection] booleans to handle runtime type checking.

e.g. this works:

  bool isFeatureCollection = instanceof(geoJSON, FeatureCollection);
  bool isFeature = instanceof(geoJSON, Feature);
  int stop = 1;
  if (isFeatureCollection) {
    stop = (geoJSON as FeatureCollection).features.length;
  }

but then further down we have things like this that do not work:

  for (var i = 0; i < stop; i++) {
    geometryMaybeCollection = (isFeatureCollection
        ? geoJSON.features[i].geometry
        : (isFeature ? geoJSON.geometry : geoJSON));
    featureProperties = (isFeatureCollection
        ? geoJSON.features[i].properties
        : (isFeature ? geoJSON.properties : {}));
    featureBBox = (isFeatureCollection
        ? geoJSON.features[i].bbox
        : (isFeature ? geoJSON.bbox : null));
    featureId = (isFeatureCollection
        ? geoJSON.features[i].id
        : (isFeature ? geoJSON.id : null));
    isGeometryCollection = (geometryMaybeCollection != null)
        ? geometryMaybeCollection.type == GeoJSONObjectTypes.geometryCollection
        : false;
...

since features is only available on the subclass FeatureCollection and not on the abstract GeoJSONObject class :-(

I think I'll just go ahead and refactor this code to meet our intended purpose, be more readable, yet diverge from simple 1:1 implementation mapping from turf.js

lukas-h commented 2 years ago

@baparham That's a little bit more difficult than that..

Not all subtypes of GeoJSONObject should have the same attributes following the RFC standard. For example Point, Polygon, etc. have coordinates, GeometryCollection has geometries, Feature has geometry, FeatureCollection has features.

Maybe we need to work more with type casting as or checking is for subclasses of GeoJSONObject..

baparham commented 2 years ago

I think it works to simply trust the type field and then cast as needed using the as operator. I tried to make a simple proof of concept, but my gh credentials locally are blocking me from pushing 🙄 so give me a couple more minutes, haha

baparham commented 2 years ago

what about this method? https://github.com/dartclub/turf_dart/pull/29

It's soooo ugly and needs to be refactored to use more focused helper functions I think, but to test the concept, I think it works.

baparham commented 2 years ago

here you can see all the classes and their inheritance in a tree diagram: GeoJSON Polymorphism (1)

Regarding naming, does GeometryObject and Geometry work as replacements for Geometry and GeometryType (respectively) perhaps to try and more closely parallel the GeoJSONObject?

[edit:] I mean, I am not really a fan of redundant naming conventions like Object and Type but we have to differentiate these higher order types somehow

lukas-h commented 2 years ago

I see why the naming can be a little bit confusing. We should definitely think of a naming convention.

Background: the RFC standard defines different groups of GeoJSON object types:

tobrun commented 2 years ago

Finally getting back to my original attempt of implementing coordEach to unblock the other APIs. I have it most of it ported from JS implementation but I'm noticing a difference in setup between Dart vs JS setup of geomEach. Am I correct to assume, we rather want the _ShortCircuit approach as with geomEach? and I should drop the JS specific optimizations as commented in This logic may look a little weird?

lukas-h commented 2 years ago

@tobrun Maybe let's first implement it like the geomEach. But later on we could test the performance of different implementation in a benchmark suite, as proposed in #32

baparham commented 2 years ago

I'll get started on flattenEach for now

lukas-h commented 2 years ago

I have a proposal:

To have all the meta functions like as independent / standalone functions, seems to me very "non-OOP", not "Dart-like", but instead like a more common style for JavaScript.

My idea is, to glue the meta functions (at for least those, that actually make sense) to the GeoJSON classes directly.

For the user, the API could look like this: 

var featColl = FeatureCollection.fromJson(json);

featColl.geomEach(
// ...
);

I started to implement this for the geomEach function in two different ways:

1) directly inside each of the classes inheriting from GeoJSONObject in geojson.dart: https://github.com/dartclub/turf_dart/blob/b02af15f456334027ebccfad92f854a5d273a82b/lib/src/geojson.dart#L68

2) as extension methods on all of the classes in this file: https://github.com/dartclub/turf_dart/blob/b02af15f456334027ebccfad92f854a5d273a82b/lib/src/geomeach_extension.dart#L6

You can see the changes in this commit: https://github.com/dartclub/turf_dart/commit/b02af15f456334027ebccfad92f854a5d273a82b

It's not yet debugged, and there could still be some hurdles with the implementation, but it already illustrates my idea very well.

Please let me know what you think!

@baparham @tobrun

tobrun commented 2 years ago

To have all the meta functions like as independent / standalone functions, seems to me very "non-OOP", not "Dart-like", but instead like a more common style for JavaScript.

I like the idea, the only things I could counter to this proposal are:

I don't think these reasons justify not doing it but I did want to provide that angle as input.

as extension methods on all of the classes in this file

I would vote for an extension approach. In the past I had libraries that were dependent on the java geojson implementation but weren't allowed to pull in turf-java due to binary size constraints. Having them exposed as extension functions will give us the flexibility to pull out geojson into an independent library and have a separate extension function library to provide turf functionality. Are there any downsides to use extension functions?

baparham commented 2 years ago

I agree with @tobrun that we don't want to arbitrarily pollute the namespace with stuff people may not care about, and your extensions approach seems to address this concern directly, that if one wants to use the syntactic sugar, they can import the extensions.

I am not very familiar with extensions as a concept, but I do not immediately see downsides. Perhaps if someone is copying example code that takes advantage of the extensions, they may not do the extra import needed, and I don't know if their IDE would suggest such an import.

lukas-h commented 2 years ago

I totally agree with both of you! Let’s keep the GeoJSON object representation in geojson.dart slim!

But there’s one catch with the extension method approach: We can’t add method overrides of a function declaration (e.g. void geomEach…);) in GeoJSONObject to each of the extensions. We can only put concrete method implementations into the extensions. This causes some weird type casting: as pointed out by @baparham here: https://github.com/dartclub/turf_dart/commit/b02af15f456334027ebccfad92f854a5d273a82b#r61986415

lukas-h commented 2 years ago

Are there any downsides to use extension functions?

baparham commented 2 years ago

Is there a way to use the actual implementation via the extension method?

import 'package:turf/src/meta.dart';
import 'package:turf/helpers.dart';

extension GeomEachGeoJSONObject on GeoJSONObject {
  void geomEachImpl2(GeomEachCallback callback) {
    geomEach(this, callback);
  }
}
baparham commented 2 years ago

I don't like that having the implementation in two places

lukas-h commented 2 years ago

I don't like that having the implementation in two places

Totally agree. Tried to incorporate that in this commit: https://github.com/dartclub/turf_dart/commit/32433250d1a4bb57d51cb8194f867985e4dbf88a

Is there a way to use the actual implementation via the extension method?

import 'package:turf/src/meta.dart';
import 'package:turf/helpers.dart';

extension GeomEachGeoJSONObject on GeoJSONObject {
  void geomEachImpl2(GeomEachCallback callback) {
    geomEach(this, callback);
  }
}

I'm not sure, if your (@baparham) idea here using the geomEach top level function or my if implementation in https://github.com/dartclub/turf_dart/commit/32433250d1a4bb57d51cb8194f867985e4dbf88a is better.

lukas-h commented 2 years ago
  • [ ] getCoord
  • [ ] getCoords
  • [ ] getGeom
  • [ ] getType

what about the other functions inside turf invariant? I guess @baparham you took the tasks from the README. I think, we should update the README, because since then, new modules have been created

In my opinion, most of the functions in turf invariant are not even needed in Dart, because of the better type system. For example getGeom, which just takes a Feature as a param and returns its geometry.

baparham commented 2 years ago

Yeah those sound like type guards that aren't needed in a typed language, I agree.

On Sun, Dec 19, 2021, 5:55 PM Lukas Himsel @.***> wrote:

  • getCoord
  • getCoords
  • getGeom
  • getType

what about the other functions inside turf invariant?

https://github.com/Turfjs/turf/tree/master/packages/turf-invariant

In my opinion, some of them are not even needed in Dart, because of the better type system. For example getGeom which just takes a Feature as a param and returns its geometry.

— Reply to this email directly, view it on GitHub https://github.com/dartclub/turf_dart/issues/12#issuecomment-997424857, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFFP3P3PRRFR2QTFWUHHC3URYE65ANCNFSM4UXYWOUQ . You are receiving this because you were mentioned.Message ID: @.***>

lukas-h commented 2 years ago

Will close this, because we are done with this milestone 🎉