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

feat: all_shortest_paths() returns the vertex list in both res and vpaths components #930

Merged
merged 1 commit into from Nov 9, 2023

Conversation

krlmlr
Copy link
Contributor

@krlmlr krlmlr commented Oct 29, 2023

Are we doing this only because in 0.10 there will be an edges component?

@krlmlr krlmlr requested a review from ntamas October 29, 2023 20:09
@szhorvat
Copy link
Member

Yes.

But I am not sure that vertices ans edges are the best names. We want consistency between all shortest path functions. Others have vpath and epath instead. functions.yaml contains vertices and edges. Whatever choice is made, it should be consistent between the different path functions.

@krlmlr
Copy link
Contributor Author

krlmlr commented Oct 29, 2023

Do we aim for consistency across all target languages?

@szhorvat
Copy link
Member

Do we aim for consistency across all target languages?

No, personally I am against that.

I was referring to consistency between different functions in R.

@krlmlr
Copy link
Contributor Author

krlmlr commented Oct 30, 2023

So, the plan would be to change all instances of $vertices to $vpath, and $edges to $epath, where appropriate?

@krlmlr krlmlr force-pushed the f-all-shortest-paths-vertices branch from 366ee78 to f3e4e5e Compare October 30, 2023 08:31
@krlmlr
Copy link
Contributor Author

krlmlr commented Oct 30, 2023

all_shortest_paths() is the only relevant instance. Changed to $vpaths because it's a list of vpath-s.

@krlmlr
Copy link
Contributor Author

krlmlr commented Oct 30, 2023

Conversely, we want an epaths component too.

@krlmlr krlmlr changed the title feat: all_shortest_paths() returns the vertex list in both res and vertices components feat: all_shortest_paths() returns the vertex list in both res and vpaths components Oct 30, 2023
@szhorvat
Copy link
Member

szhorvat commented Oct 30, 2023

Conversely, we want an epaths component too.

This we cannot have in 1.6.0, as 0.9 does not provide the edge path, and the edge path cannot be reconstructed from the vertex path in case of multigraphs. That is precisely why it was added as a separate component of the result. It seems to me that we cannot do more in this PR.

@krlmlr
Copy link
Contributor Author

krlmlr commented Oct 30, 2023

Yeah, let's do $epaths for 0.10.

@Antonov548 Antonov548 mentioned this pull request Oct 31, 2023
41 tasks
@krlmlr krlmlr merged commit 077b00e into main Nov 9, 2023
12 checks passed
@krlmlr krlmlr deleted the f-all-shortest-paths-vertices branch November 9, 2023 19:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants