Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GSoC 2023: Yige Huang Week 2 #299

Merged
merged 8 commits into from
Jun 10, 2023
Merged

Conversation

squarege
Copy link
Contributor

@squarege squarege commented Jun 9, 2023

Changes proposed in this pull request:

  • standardizing output: the single vertex overload include column start_vid.
  • changing optional parameter driving_side:
    • from:
      Value in [r, l, b] indicating if the driving side is:
      r for right driving side.
      l for left driving side.
      b for both.
    • to:
      Value in [r, R, L, l] indicating if the driving side is:
      r,R for right driving side
      l,L for left driving side
      Any other value (including NULL) will be considered as r
  • updating pgtap

@pgRouting/admins

@squarege squarege added the yige-2023 Yige's GSoC 2023 project label Jun 9, 2023
@squarege squarege merged commit 811d1f0 into pgRouting:yige-2023 Jun 10, 2023
10 checks passed
@@ -16,13 +16,6 @@ SELECT * FROM pgr_withPointsDD(
driving_side => 'l',
equicost => true);
/* -- q4 */
SELECT * FROM pgr_withPointsDD(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not delete the query its been used in the documentation.
Put it back.

@@ -34,7 +34,7 @@ CREATE FUNCTION _pgr_withPointsDD(
distance FLOAT,

directed BOOLEAN DEFAULT true,
driving_side CHAR DEFAULT 'b',
driving_side CHAR DEFAULT 'r',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still like the default value to be r, About getting rid of b we need to discuss

@@ -20,53 +20,17 @@ Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.

BEGIN;

SELECT PLAN(6);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do not change pgtap tests at this time, put it back as it was ...
Of course pgtap will fail, it is expected.

CREATE FUNCTION pgr_withPointsDD(
TEXT, --edges_sql (required)
TEXT, -- points_sql (required)
ANYARRAY, -- from_vid (required)
FLOAT, -- distance (required)

directed BOOLEAN DEFAULT true,
driving_side CHAR DEFAULT 'b',
driving_side CHAR DEFAULT 'r',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a tricky change,
Basically the default for versions 3.5 & under is band the default for 3.6 and after is r
For the moment lest leave it like it is here, but open an issue "reminder of default value" in your fork and put a link to this comment

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue for reminder: squarege#5

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #300

@@ -54,6 +54,9 @@ void process(
Path_rt **result_tuples,
size_t *result_count) {
driving_side[0] = estimate_drivingSide(driving_side[0]);
if (driving_side[0] != 'r' && driving_side[0] != 'l') {
Copy link
Member

@cvvergara cvvergara Jun 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not the best place to check, did you analyze what estimate_drivingSide is doing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I did. estimate_drivingSide converts the input character to lowercase, and returns r, l or b (if it is neither r nor l)
As the estimate_drivingSide function is also used by other withPoints functions, modifying it will have a greater impact,
so I made modifications above with reference to the following code:

driving_side[0] = estimate_drivingSide(driving_side[0]);
if (driving_side[0] != 'r' && driving_side[0] != 'l') {
driving_side[0] = 'l';
}

But if the default for versions 3.6 and after is r, maybe I can change estimate_drivingSide?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #300

@squarege squarege changed the title GSoC 2023: Yige Huang week 2 GSoC 2023: Yige Huang Week 2 Aug 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
yige-2023 Yige's GSoC 2023 project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants