Better dot density - Githubissues

stuartlynn commented 7 years ago

[ ] All declared geometries are geometry(Geometry, 4326) for general geoms, or geometry(Point, 4326)
[x] Existing functions in crankshaft python library called from the extension are kept at least from version N to version N+1 (to avoid breakage during upgrades).
[ ] Docs for public-facing functions are written
[x] New functions follow the naming conventions: CDB_NameOfFunction. Where internal functions begin with an underscore _.
[x] If appropriate, new functions accepts an arbitrary query as an input (see Crankshaft Issue #6 for more information)

Fixes some issues with DotDensity:

Increases performance
Works properly with geoms with holes in them
Does proper equal area assignment of points

stuartlynn commented 7 years ago

@iriberri not sure why the tests are failing here. Travis logs say:

The command "sudo apt-get -y install postgresql-9.5=9.5.2-2ubuntu1" failed and exited with 100 during .

ethervoid commented 7 years ago

@stuartlynn @iriberri is not longer a Carto employee :(. I'll fix it for you

stuartlynn commented 7 years ago

Sorry @iriberri and @ethervoid, I was running exhausted on fumes when I submitted that comment. Wasn't thinking sorry :-(

@ethervoid if that is good then I think we are ready to go ahead and merge this

ethervoid commented 7 years ago

What about the first bullet point?

ethervoid commented 7 years ago

It looks like we have a problem with Travis, tomorrow I'll give a look.

On the other hand, I've seen that you're removing a public function to create another one following the PostGIS convention but we can't delete public functions because that belongs to the public API and we can't remove it like that. We have to deprecate them and so on.

The best way to do it is to transform the old function into a wrapper for the new function, that is make the old function call the new function without changing the "prototype" (function name, parameters and return type)

stuartlynn commented 7 years ago

Hey man. The function signature changed cause I was adding the geometry types. I wonder if just this once we should leave the function deffinition without the geometry types so we dont have to worry about the conflict?

ethervoid commented 7 years ago

We should leave the old function that internally calls the new function (like a wrapper). Once we know that the old function is not used anymore, we could remove it safely

stuartlynn commented 7 years ago

OK sounds good. Will fix that up today.

Cheers

andy-esch commented 7 years ago

We should retire the cdb_dot_density after this PR is merged. @rafatower, do we have a process for deprecating functions?

@stuartlynn, I don't see the docs in this PR. Did they not get committed? I'm happy to write them if needs be.

stuartlynn commented 7 years ago

No docs but we still need to do the wrapping function. @ohasselblad if you don't mind doing that and throwing together some docs that would be great.

rafatower commented 7 years ago

We should retire the cdb_dot_density after this PR is merged. @rafatower, do we have a process for deprecating functions?

@ohasselblad intentionally we don't have such a process. Bear with me and I'll explain why:

That is a public function as per the naming convention. Also note that we purposefully decided not to deal with API compatibility breakage. See https://github.com/CartoDB/crankshaft/blob/develop/RELEASE.md#some-remarks

Much of the process is automated under those assumptions in the release doc. But it is not just automation...

Imagine that some user's client code is using cdb_dot_density function. If we removed the function, then the user database would not be able to upgrade crankshaft extension, so getting them effectively locked into current version.

Having a user locked into a particular version is a support and maintenance issue. It might well happen, of course, that no user is using that function but this can be generalized to any public function and I do believe checking individual cases is not an option at the speed we want to move.

We could also replace the body of the function to just return NULL and raise a warning. But then something that used to work would suddenly stop working for the user.

Or we could just add the warning with a deprecation note. We usually support API's for one year minimum.

Imagine for a moment that you are that user. What would you prefer?

My point is that we shall assume public API's will have to be supported for long periods and be careful when choosing what's private and what's public.

Hope this sheds some lights.

andy-esch commented 7 years ago

Thanks, @rafatower, super helpful.

I'll make sure it's marked 'deprecated' in the docs, but I also want to address that it probably should be updated from the updates in this PR since it makes several important improvements.

Functionally it will work the same as CDB_DotDensity, so I could simply do RETURN CDB_DotDensity(...) as the internals of cdb_dot_density to avoid having to maintain two functions that do the same thing. Does that sound like a good solution?

rafatower commented 7 years ago

A pretty good one, indeed. Thanks! :)

andy-esch commented 7 years ago

@mehak-sachdeva, one change that needs to happen is the function name that I mentioned in my last comment.

mehak-sachdeva commented 7 years ago

@stuartlynn @andy-esch
Added the old signature function back - wrapping and returning the new function from in there. Please let me know if I forgot to add/edit something. Thank you!

andy-esch commented 6 years ago

PostGIS has a ST_GeneratePoints function that could probably be used in place of the FOR LOOP in the dot density function.

More here: http://www.postgisday.rocks/query/2017/11/16/generating-random-points-inside-a-geometry.html

CartoDB / crankshaft

Better dot density #148