search

Fil / d3-geo-voronoi

Voronoi / Delaunay tessellations on the sphere
ISC License
229 stars 24 forks source link

geo_edges: Use the more semantically meaningful Set #26

Closed martinfrances107 closed 3 years ago

martinfrances107 commented 3 years ago

The last 2 commits have replaced the generic javascript object {} data objects with Map and Set data structures.

This is the last issue of that form.

I am about to attach a PR which show one of two possible approaches

The PR needs discussion as approach one has possible performance implications where as options two involves a backwards compatible breaking change.

So this set of judgement calls needs careful review.

In this PR to keep the return of geo_edges() to be an array I was forced chain and Array.from() into a .map()

return Array.from(_index).map(d => d.split("-").map(Number)); the JIT compiler may look at the chain of language primitives and do the correct thing - looping only once.

but on the face of it the PR introduces a "double looping structure."

My preferred solution is option two in which the return type of array becomes an iterator

... Obviously this would break all existing code bases in the wild.

making things faster is a concern https://github.com/Fil/d3-geo-voronoi/issues/7

Maybe we should just leave geo_edges alone Maybe the apparent double looping is acceptable. Maybe we need to wait until the next major version number update?

martinfrances107 commented 3 years ago

After a nights sleep ...

I think the ideal backward compatibility breaking solution would just simply return the Set. That would preserve the semantic meaning ...

That is

The function geo_edges() returns more than an array of edges .. it returns a Set of edges ( The elements are guaranteed to be be unique/ free of duplicates. )

Fil commented 3 years ago

Alas… it wouldn't be so great, since Javascript sets do not work as expected with arrays, because two "identical" arrays are not equal: new Set([[0], [0]]).size === 2. This is why we do the awful "join to string / split string" thing. Because of this I think it's better to keep the result as an array. (I welcome your comments if you disagree :) )

martinfrances107 commented 3 years ago

Thank you for your reasoning ... I am sure now that you are correct.