pgRouting / pgrouting

Repository contains pgRouting library. Development branch is "develop", stable branch is "master"
https://pgrouting.org
GNU General Public License v2.0
1.13k stars 362 forks source link

withPoints family of functions #553

Closed cvvergara closed 7 years ago

cvvergara commented 8 years ago

proposed functions withPoints family of functions

Please leave your comments, suggestions, documentation comments about any of the with points family of functions.

robe2 commented 8 years ago

Clarify that if a point has a fraction of 0 or 1, it should not have a pid.

For example I had this:


SELECT *
FROM pgr_withPointsDD(
        'SELECT gid As id, source, target, 
            cost_s AS cost, reverse_cost_s AS reverse_cost 
            FROM ospr.ways',
        'SELECT osm_id AS pid, edge_id, fraction 
            FROM dc_hospitals',
   15287, 5*60, 
      driving_side := 'b', details := true) 
;

and it didn't work. The 15287 is a node that falls right on a hospital.

Had to change to this:

SELECT * 
    FROM pgr_withPointsDD(
        'SELECT gid As id, source, target, 
            cost_s AS cost, reverse_cost_s AS reverse_cost 
            FROM ospr.ways',
        'SELECT pid, edge_id, fraction 
            FROM dc_hospitals WHERE pid < 0',
     15287, 5*60, 
      driving_side := 'b', details := true);

Should also clarify that if you have a point that has 0 or 1 fraction, you should not create a pid for it. It will take on the node from your edges data, and if you need to know the pid, you need to account for this.

woodbri commented 8 years ago

Thw name withPoints has always bothered me because it is not clear what I'm getting withPoints. Is it dijkstraWithPoints or drivingDistanceWithPoints or trspWithPoints or pizzaWithPoints, or maybe it is just routingWithPoints, etc. I think it is like an abbreviation for somethingWithPoints and we should probably qualify what the something is.

Regardless, withPoints is not descriptive enough for my feeble brain to get a good handle on what is does or how it might be different than dijkstra. Also withPoints does not make it clear if the points are via points used in planning the route or extra points of interest that are only reported if the route passes through them. I think we can be a little more verbose in naming this family of functions to make it more clear. I'm embracing your idea of make variable and function names that are self documenting and avoiding abbreviations and names that are do not provide much meaning or context. :)

cvvergara commented 8 years ago

@woodbri I am sure you read the whole documentation of the what I called "with points family of functions", So, can you please be more specific on the naming of the:

The good thing about not making the functions not official to the release is that: name can be changed so something more understandable.

cvvergara commented 8 years ago

added a "not" in previous comment

woodbri commented 8 years ago

So it makes sense (to me) to use names like:

for these functions, because it is the same command but with slightly different input. If fact, why are we not just overloading the command name so the function name remains the same and only the arguments change?

dkastl commented 8 years ago

I must admit Steve, that withPoints sounds a bit unclear, what is actually does ... because I don't read the documentation carefully ;-) But I like the withPoints functionality, because I tried something similar some time ago with a Wrapper function and failed.

The problem: I can't propose a better name. However, I think having function names, that tell what they do without having to look at the documentation isn't a bad thing.

I remember, someone mentioned once, that "function overloading" is bad. I don't remember, who and why. But I actually like overloading. Are there any good reasons not to use the same function names with different arguments?

cvvergara commented 8 years ago

This are all the signatures

pgr_withPoints(edges_sql, points_sql, start_vid, end_vid)
pgr_withPoints(edges_sql, points_sql, start_vid, end_vid, directed, driving_side, details)
pgr_withPoints(edges_sql, points_sql, start_vid, end_vids, directed, driving_side, details)
pgr_withPoints(edges_sql, points_sql, start_vids, end_vid, directed, driving_side, details)
pgr_withPoints(edges_sql, points_sql, start_vids, end_vids, directed, driving_side, details)
pgr_withPointsCost(edges_sql, points_sql, start_vid, end_vid)
pgr_withPointsCost(edges_sql, points_sql, start_vid, end_vid, directed, driving_side)
pgr_withPointsCost(edges_sql, points_sql, start_vid, end_vids, directed, driving_side)
pgr_withPointsCost(edges_sql, points_sql, start_vids, end_vid, directed, driving_side)
pgr_withPointsCost(edges_sql, points_sql, start_vids, end_vids, directed, driving_side)
pgr_withPointsKSP(edges_sql, points_sql, start_pid, end_pid, K)
pgr_withPointsKSP(edges_sql, points_sql, start_pid, end_pid, K, directed, heap_paths, driving_side, details)
pgr_withPointsDD(edges_sql, points_sql, start_vid, distance)
pgr_withPointsDD(edges_sql, points_sql, start_vid, distance, directed, driving_side, details)
pgr_withPointsDD(edges_sql, points_sql, start_vids, distance, directed, driving_side, details, equicost)

So, lets make a overloaded pgr_FOO that uses only the parameters, but now, there can not be optional parameters

pgr_FOO(edges_sql, points_sql, start_vid, end_vid)

because that is withPoints or withPointsCost ????

so the signatures look like:

pgr_FOO(edges_sql, points_sql, start_vid, end_vid, directed, driving_side, details)
pgr_FOO(edges_sql, points_sql, start_vid, end_vids, directed, driving_side, details)
pgr_FOO(edges_sql, points_sql, start_vids, end_vid, directed, driving_side, details)
pgr_FOO(edges_sql, points_sql, start_vids, end_vids, directed, driving_side, details)
pgr_FOO(edges_sql, points_sql, start_vid, end_vid, directed, driving_side)
pgr_FOO(edges_sql, points_sql, start_vid, end_vids, directed, driving_side)
pgr_FOO(edges_sql, points_sql, start_vids, end_vid, directed, driving_side)
pgr_FOO(edges_sql, points_sql, start_vids, end_vids, directed, driving_side)
pgr_FOO(edges_sql, points_sql, start_pid, end_pid, K, directed, heap_paths, driving_side, details)
pgr_FOO(edges_sql, points_sql, start_vid, distance, directed, driving_side, details)
pgr_FOO(edges_sql, points_sql, start_vids, distance, directed, driving_side, details, equicost)

Now I have a not so complex query:

SELECT * 
    FROM pgr_FOO(
        'SELECT gid As id, source, target, 
            cost_s AS cost, reverse_cost_s AS reverse_cost 
            FROM ospr.ways',
        'SELECT * 
            FROM pointsofinterest WHERE pid < 0',
     15287, 300, 
      true, 'b', true);

and this other not so complex query:

SELECT * 
    FROM pgr_FOO(
        'SELECT gid As id, source, target, 
            cost_s AS cost, reverse_cost_s AS reverse_cost 
            FROM ospr.ways',
        'SELECT * 
            FROM pointsofinterest WHERE pid < 0',
     15287, 300, 
      true, 'b');
cvvergara commented 8 years ago

http://www.postgresql.org/docs/9.3/static/xfunc-overload.html

cvvergara commented 8 years ago

Following the previous big comment: What are the not so complex queries calculating?

dkastl commented 8 years ago

OK, withPoints or withPointsCost may be a reason, why overloading is not so good idea. I'm a bit worried about having to double the queries for each algorithm as we do now.

Well, maybe with v3 we will find a different way to handle this.

cvvergara commented 8 years ago

So, lets think ahead, suppose pgr_astar is working fine: pgr_astar(one to one) pgr_astar(one to many) pgr_astar(many to one) pgr_astar(one to many) pgr_astarCost(one to one) pgr_astarCost(one to many) pgr_astarCost(many to one) pgr_astarCost(one to many) pgr_astarKSP(...) pgr_astarDD(one starting) pgr_astarDD(many starting) pgr_astarVIA(..)

pgr_astarWithPoints(one to one) pgr_astarWithPoints(one to many) pgr_astarWithPoints(many to one) pgr_astarWithPoints(one to many) pgr_astarWithPointsCost(one to one) pgr_astarWithPointsCost(one to many) pgr_astarWithPointsCost(many to one) pgr_astarWithPointsCost(one to many) pgr_astarWithPointsKSP(...) pgr_astarWithPointsDD(one starting) pgr_astarWithPointsDD(many starting) pgr_astarWithPointsVIA(..)

dkastl commented 8 years ago

Question one: why don't we make the "algorithm" an argument then? And default could be "dijkstra".

cvvergara commented 8 years ago

So, I'll rewrite the pgr_astar idea: (optional parameter can not go first) pgr_XXX( ......, algorithm:="dijkstra") pgr_Cost(......, algorithm:="dijkstra") pgr_KSP(......, algorithm:="dijkstra") pgr_drivingDistance(......, algorithm:="dijkstra") pgr_VIA(......, algorithm:="dijkstra")

pgr_WithPoints( ......, algorithm:="dijkstra") pgr_WithPointsCost(......, algorithm:="dijkstra") pgr_WithPointsKSP(......, algorithm:="dijkstra") pgr_WithPointsDD(......, algorithm:="dijkstra") pgr_WithPointsVIA(......, algorithm:="dijkstra")

Now I have a little bit more complex query:

 SELECT *
FROM pgr_alphaShape( $$SELECT v.id::int4, ST_X(v.the_geom) AS x, ST_Y(v.the_geom) As y 
        FROM pgr_drivingDistance('SELECT gid As id, source, target, 
            cost_s AS cost, reverse_cost_s AS reverse_cost 
            FROM ospr.ways',
        (SELECT n.id 
            FROM ospr.ways_vertices_pgr AS n
              ORDER BY ST_SetSRID(
                ST_Point(-77.02734, 38.97328),4326) <-> n.the_geom LIMIT 1) 
        , 20, true, "astar", 
      ) AS dd INNER JOIN ospr.ways_vertices_pgr AS v ON dd.node = v.id$$)

VS

 SELECT *
FROM pgr_alphaShape( $$SELECT v.id::int4, ST_X(v.the_geom) AS x, ST_Y(v.the_geom) As y 
        FROM pgr_astarDD('SELECT gid As id, source, target, 
            cost_s AS cost, reverse_cost_s AS reverse_cost 
            FROM ospr.ways',
        (SELECT n.id 
            FROM ospr.ways_vertices_pgr AS n
              ORDER BY ST_SetSRID(
                ST_Point(-77.02734, 38.97328),4326) <-> n.the_geom LIMIT 1) 
        , 20, true 
      ) AS dd INNER JOIN ospr.ways_vertices_pgr AS v ON dd.node = v.id$$)
cvvergara commented 8 years ago

Forgot to mention: astar edges_sql query is different than of dijkstra's edges_sql query

cvvergara commented 8 years ago
 SELECT *
FROM pgr_alphaShape( $$SELECT v.id::int4, ST_X(v.the_geom) AS x, ST_Y(v.the_geom) As y 
        FROM pgr_drivingDistance('SELECT gid As id, source, target, 
            cost_s AS cost, reverse_cost_s AS reverse_cost ,
            x1, y1, x2, y2
            FROM ospr.ways',
        (SELECT n.id 
            FROM ospr.ways_vertices_pgr AS n
              ORDER BY ST_SetSRID(
                ST_Point(-77.02734, 38.97328),4326) <-> n.the_geom LIMIT 1) 
        , 20, true
      ) AS dd INNER JOIN ospr.ways_vertices_pgr AS v ON dd.node = v.id$$)

that defaults to dijkstra, and would be an error

robe2 commented 8 years ago

I noticed that withPointsDD array version returns start_vid whereas pgr_drivingDistance array version returns from_v. I think the withPoints should be changed to be consistent

robe2 commented 8 years ago

Regarding how overloading can get a little out of hand. In PostGIS 2.1 we decided to merge all the ST_MapAlgebra (neighborhood, multi raster, int one name). Sounded like a good idea at teh time.

now our documentaton looks really scary:

http://postgis.net/docs/manual-2.2/RT_ST_MapAlgebra.html

robe2 commented 8 years ago

I also noticed - pgr_withPointsDD in testing doesn't have a equidistance := true option. Would be useful to have that.

cvvergara commented 7 years ago

I'll close this issue as the latest comment on the topic was 9 months ago. Any problem with withPoints open a new issue