Turfjs / turf

A modular geospatial engine written in JavaScript and TypeScript
https://turfjs.org/
MIT License
9.26k stars 937 forks source link

Add coordMap function to complement coordEach #1438

Open cyrilchapon opened 6 years ago

cyrilchapon commented 6 years ago

Explaination

Hey :)

Yesterday, searching for a tool to loop and map over coordinates, I did not succeed to find one.

The idea was basically simple : loop over all coordinates, and map each of them with a new item (new position, i.e); but the key part is leaving the whole geojson object untouched

I can think of 3 example use-cases, including mines :

I found coordsEach (which just loop), coordsReduce (which would suffice to recompose a new geojson, with indexes and stuff, but would not be very readable), and perliedman/reproject (which is too linked with reprojection stuff, and provide a too-rough cloning strategy IMHO). FYI I opened an issue there to propose extraction of "mapping coordinates logic" in another repo.

I ended up implementing it myself, but I think it could be a great addition to @turfjs 😄

Here is it : traverse-geojson

I used some gjtk tools, and the implementation is very basic. I'd like to use @turfjs formalism and helpers (invariants, etc.), but I'm not really used to it.


API

coordsMap(input, transformer) => returns output (same shape as input)

Params


Questions :

Is that a good idea ?

Could it be integrated into turf ?

What about also featuresMap, which would basically do the same stuff, but with features ?

stebogit commented 6 years ago

Hey @cyrilchapon, thanks for the suggestion. I am personally a supporter of the idea of completing the already available "Array like" functions we have in Turf, i.e. adding .coordMap and/or .featureMap methods. 👍 There is an open discussion about a similar topic.

Feel free to send a PR and we'll see if we can reach the quorum to merge it to master. 🤞

Note: we (namely @rowanwins) are currently working on a quite significant renovation of the entire repo, so it might take a while before we take in account new modules/functions.

cyrilchapon commented 6 years ago

Well, it may be a good start to do it in the formalism you want to introduce inside v7, and maybe PR directly there ?

I need help on it though. Maybe another contributor than yourself can help me accomplish this ?

rowanwins commented 6 years ago

Hey @cyrilchapon

So I'm just trying to understand why the existing coordEach isn't suitable? So for example we use coordEach in our truncate module here. We have an option on truncate called mutate which specifies whether you mutate the input or not.

tmcw commented 6 years ago

Untested, but this would be the general direction of implementing each suggestion. And, yes, this mutates the data, but... just clone the data ahead of time, which is just the same as what a function would do internally.

Reproject a whole GeoJSON object

coordEach(object, coord => {
  const p = reproject(coord);
  coord.length = 0;
  coord.push(...p);
});

Remove z ([lng, lat, z]) from coordinates in a whole, dirty, GeoJSON object

coordEach(object, coord => coord.pop());

// safer if only some have a z
coordEach(object, coord => {
  const [x, y] = coord;
  coord.length = 0;
  coord.push(x, y);
});

Round coordinates to 6 decimal places into a whole GeoJSON object

coordEach(object, coord => {
  for (let i = 0; i < coord.length; i++) coord[i] = parseFloat(coord[i].toFixed(6));
});
cyrilchapon commented 6 years ago

Hey @rowanwins and @tmcw, basically because those algorithms are mutative and not really readable.

It's 2018, ES6 is here, and non-mutative mutations are well known now, and purity should not be seen like a compromise to performance.. So basically "const input = options.mutate ? geojson : deepClone(geojson)" then performing a rough mutating algorithm doesn't seem very ok to me.

The question was not about "is that possible with the current state of art", the question was "would it be a great addition to the lib ?"

If you want you can check my implementation inside aforementioned repo, you'll understand

rowanwins commented 6 years ago

Hey @cyrilchapon

the question was "would it be a great addition to the lib ?"

I think we're just trying to understand why it would be a great addition. To be honest I'm struggling to see a little bit why it is. We've got an existing module that can be used to pretty much do the same thing, admittedly using a slightly different approach. I've taken a look through your repo and it does make sense, I'm not arguing that. The main downside I see of your proposal is that it adds more surface to the turf codebase to test and maintain when we can already achieve much the same result.

It's 2018, ES6 is here

Currently TurfJS doesn't use ES6 features, we don't use babel in transpiling any code. That's not to say it wont happen, we are for instance currently implementing ES6 modules.

I don't think we're trying to be difficult so sorry if it feels like that.

cyrilchapon commented 6 years ago

Hey @rowanwins, sorry for being a bit difficult to, bad night 😜

We've got an existing module that can be used to pretty much do the same thing, admittedly using a slightly different approach

The main downside I see of your proposal is that it adds more surface to the turf codebase to test and maintain when we can already achieve much the same result.

Well, what I understood about turf.js, and I've been using this for like 2 years, importing like 60% of it's modules; the point seems to be abstraction, centralisation and simplification of complex geographic stuff (usually already available in various modules accross the web)

Think of it, we have coordsEach, okay we can loop. Why did someone develop coordsReduce on the first place ? After all, the same thing can be accomplished with coordsEach... but (except maybe arguing that coordsReduce is used in other turf modules), the concept of reducing is something one Javascript developer typically use way less than the concept of mapping

If coordsReduce was a good addition, I think we can argue that coordsMap would not only be a consistent choice to follow that logic (I.E kinda reimplementing "array functions" deeply in geojson objects); but could also be more useful.

Currently TurfJS doesn't use ES6 features, we don't use babel in transpiling any code. That's not to say it wont happen, we are for instance currently implementing ES6 modules.

This is another topic indeed, and I could argue in ES6 (even ES2017) stuff but maybe somewhere else than in that issue. There are 2 discussions here :

The point is "the syntax when using it" is part of "the feature", and not "the syntax of implementation" if you follow me. I, being a little bit narrow minded, am not found of :

coordEach(object, coord => {
  for (let i = 0; i < coord.length; i++) coord[i] = parseFloat(coord[i].toFixed(6));
})

By example

Arguing that the API, and readability and syntax of usage is part of the feature, see

Reproject a whole GeoJSON object

const reprojected = coordsMap(
  geojson,
  coords => reproject(coords)
)

Remove z ([lng, lat, z]) from coordinates in a whole, dirty, GeoJSON object

const reprojected = coordsMap(geojson, coords => coords.slice(0, -1))

// safer if only some have a z
const reprojected = coordsMap(geojson, coords => coords.slice(0, 1))

Round coordinates to 6 decimal places into a whole GeoJSON object

const reprojected = coordsMap(
  geojson,
  coords => coords.map(
    coord => coord.toFixed(6)
  )
)
tmcw commented 6 years ago

I'm the original author of turf-meta, so, I'll at least explain that part:

turf-meta wasn't initially designed as a convenience module, or a simple abstraction. turf had a lot of modules that claimed to take 'geojson' as input, but in reality they only accepted one type of geojson (like a featurecollection), or weren't able to handle certain atypical forms (like null geometries). turf-meta was designed and written to be a near-zero overhead module that ensured that people could write turf modules as robust by default, instead of slowly discovering the various other forms of geojson that don't immediately come to mind.

This original intent is why, for example, I've typically opposed efforts to improve the ergonomics of turf-meta, unless they can clearly be accomplished without any performance penalty. A performance problem in turf-meta would ripple out to the rest of turf, making lots of other functions slower. Which would be true of any effort at immutability: mapping an array into another array requires more memory and is slower than iterating and mutating an existing array, and there's no way JavaScript will be able to change that, ES9 or beyond.


Anyway, not to get all opinionated about this stuff: something like coordMap would be fine if a majority of folks want it and someone's interested in maintaining it. But it probably shouldn't be part of turf-meta, because its intent and behavior would be different, and probably shouldn't be named coordMap, because it's more like transformCoords or something - given the signatures of coordEach and coordReduce, you'd expect it to return coords, not a geojson object.

cyrilchapon commented 6 years ago

Hey @tmcw, some interesting stuff here 🙂

I basically agree with all of that, and am very aware of the performance downside of immutability when talking about processing.

About performance, It's a deep topic, but there are use-cases where immutability is very mandatory, and I could even argue that there are use-cases where the performance impact of mutating every references in the object is worse than mutating it. Yes, basically, copying (cloning) something engages more memory than mutating it; but I guess you're thinking about performance, with some brutal, one time, processing of a big geojson where that "copy" and number of iterations is the main concern. But think of folks maybe using that in the frontend. I usually push geojson references in redux stores that go down to react components. In such environnements, optimization of rendering is basically done by checking references changes. In there, mutating just doesn't work, and if the alternative is "to break all the references and clone the whole stuff" is the worse thing to do, causes some massive re-rendering. I personally often face this case with react mapbox wrappers. Having some non-mutative alternatives opens the door to many things, many optimizations that go further than the single performance concern of processing and I suspect this is why so many turf functions have a "mutate" option; but as I said those options deeply clone the object, and break references.

The "ES-whatyouwant" stuff was not between "mutation vs non-mutation" but more between "clone the whole thing then mutate the clone vs cleverly process non-mutating"

About this stuff landing or not in here, that's the question. I personally love turf, and am extensively using it, but am not aware of why-this-part-landed-here or the strategy of the whole thing. Though, having an external point of view is sometimes valuable and I can surely bring you that point of view : independently of "why turf-meta was designed in the first place", this is now a publicly accessible package with many small modules very efficients and useful for folks manipulating geojson. History is one thing, present is another and the API of a public module can ask herself to evolve or not to evolve, be it named "meta" or not 😋

cyrilchapon commented 6 years ago

And about the API in itself (returning geoJSON) object.. well from my external POV I understood "coordsWhatever" functions like "mimic array functions on GeoJSON objects". I kinda agree with you, but returning an array of transformed cords would be pointless, I guess (as you loose every indexes, tree logic, features)

stevage commented 1 year ago

Just jumping in to say I keep running into situations where a coordMap (or, per tmcw, transformCoord) would be handy. I'm loading some big GeoJSONs into the browser and doing some pretty heaviy handed processing on them.

What I really want to do often is write something like turf.coordMap([x, y] => [x + dx, y+dy]). The workaround with coordEach is:

turf.coordEach(coord => { coord[0] += dx; coord[1] += dy; })

but it's clunky.

An even worse case is one where I wanted to drop the z coordinate off every coordinate. Do you know off the top of your head how to shorten an array by mutating it? I had to google it.

turf.coordEach(coord => coord.pop())

is much less intuitive than: turf.coordMap(coord => [coord[0], coord[1]])

I've had 4 or 5 cases like this now. Clipping coordinates to a boundary, translating by some amount, projecting point by point, etc etc.