feat: New `k_shortest_paths()` to compute the first k shortest paths between two vertices by szhorvat · Pull Request #1028 · igraph/rigraph

szhorvat · 2023-12-28T22:29:35Z

Fixes #972 by adding k_shortest_paths().

This is a draft, but it should be reviewed now. Keep in mind that I don't actually know much R! In fact I wouldn't mind if someone took this over or helped out.

The reason why I did not add this to the doc page of shortest_paths() is that the to parameter is different: it must be a single vertex.

@maelle:

I tried to use @inheritParams here.
I'm not sure if it's normal that so many other functions' doc pages got references to this one?

@krlmlr:

I did not attempt to add an output parameter like shortest_paths has. Both edge and vertex paths are returned unconditionally.

I hope to have this much-requested feature in 2.0.0.

CC @luukvdmeer

aviator-app · 2023-12-28T22:29:38Z

Current Aviator status

Aviator will automatically update this comment as the status of the PR changes.
Comment /aviator refresh to force Aviator to re-examine your PR (or learn about other /aviator commands).

This PR was merged using Aviator.

See the real-time status of this PR on the Aviator webapp.

Use the Aviator Chrome Extension to see the status of your PR within GitHub.

szhorvat · 2023-12-28T22:44:10Z

@krlmlr This does not currently work and I need some advice on how to fix it. The reason for wrapping get_k_shortest_paths_impl() is to reorder the parameters. I could call the C function directly, and not auto-generate R code, but that seems an unnecessary amount of work.

> k_shortest_paths(make_ring(5), 1, 2, k=3)
Error in match.arg(arg = arg, choices = choices, several.ok = several.ok) : 
  'arg' must be of length 1
> traceback()
5: stop("'arg' must be of length 1")
4: match.arg(arg = arg, choices = choices, several.ok = several.ok)
3: igraph.match.arg(mode)
2: get_k_shortest_paths_impl(graph = graph, weights = weights, k = k, 
       from = from, to = to, mode = mode)
1: k_shortest_paths(make_ring(5), 1, 2, k = 3)

@ntamas Is there a stimulus feature to reorder parameters? Can this be handled in functions-R.yaml somehow? What is the purpose of specifying PARAMS in that file?

szhorvat · 2023-12-28T22:52:59Z

@ntamas OK, I found PARAM_ORDER, but that's still not sufficient to avoid having a wrapper here. I need to rename the vertex.paths and edge.paths components of the result to simply vpaths and epaths. Do we have a stimulus feature for this?

szhorvat · 2023-12-28T22:56:51Z

Never mind, I just realized that PARAM_NAMES works since these still parameters, even though output parameters.

The R code is now fully auto-generated and it works.

szhorvat · 2023-12-28T23:01:51Z

@ntamas Is there a way to set default parameter values just in R when they are not set in the C core? What if I'd like to set a default of k=1 in this function?

ntamas · 2023-12-28T23:36:09Z

Is there a way to set default parameter values just in R when they are not set in the C core?

No, not at the moment, apart from copying the entire parameter specification of the function from the C core and adding the default value there. We can implement something like this in Stimulus, although the ideal solution would be to define the default value in the C core.

krlmlr

Thanks. What would a test look like here?

krlmlr · 2023-12-29T04:34:23Z

R/aaa-auto.R

@@ -819,14 +819,14 @@ get_k_shortest_paths_impl <- function(graph, weights=NULL, k, from, to, mode=c("
  res <- .Call(R_igraph_get_k_shortest_paths, graph, weights, k, from-1, to-1, mode)


I wonder why we have -1 here but no inverse correction for the output. Is this expected?

I don't have the code in front of me right now, but I remember that there aren't any clear guidelines on where to do the +1/-1 adjustments for vertex/edge indices and sometimes it's done in the output conversion of the result. For instance, there is R_igraph_vector_int_to_SEXP to convert an igraph_vector_int_t back to R without adjustment and R_igraph_vector_int_to_SEXPp1 to convert it back with adjustments. (The p1 suffix stands for "plus one").

Ideally, there should be guidelines on where the adjustment is done. If it is easier to do in R and there are no performance implications (like an unnecessary copying of the vector), it should probably be done in R and we could get rid of the p1 functions in rinterface_extra.c.

There are performance implications, ideally we'd convert everything in C.

Dreaming big: If we could get views that cast to int and perform the "minus one" on the fly, in the C core...

krlmlr · 2023-12-29T04:50:48Z

R/aaa-auto.R

 }

-get_k_shortest_paths_impl <- function(graph, weights=NULL, k, from, to, mode=c("out", "in", "all", "total")) {
+get_k_shortest_paths_impl <- function(graph, from, to, k, weights=NULL, mode=c("out", "in", "all", "total")) {


The following would be safer:

Suggested change

get_k_shortest_paths_impl <- function(graph, from, to, k, weights=NULL, mode=c("out", "in", "all", "total")) {

get_k_shortest_paths_impl <- function(graph, from, to, ..., k, weights=NULL, mode=c("out", "in", "all", "total")) {

check_dots_empty()

This would require users to supply names for k and the other optional arguments. Positional matching makes it harder:

for users to skim the code (what does that fifth argument mean again?),

for us to add new arguments (all new ones must go at the end to maintain full compatibility, which is not always desired).

I wonder if this could be part of Stimulus, or if we create a wrapper here (and in many other places).

Stimulus does not have a way to indicate a preference for keyword arguments vs positional arguments, but I guess it could be added. Python does it by adding a lone * in the argument list (e.g., def compare(a, b, *, key=None): makes key a keyword argument only while a and b could be defined either as positional or as keyword arguments). We could adapt something similar in Stimulus if this is a concept that seems to apply to multiple languages. Languages without kwarg support could ignore the asterisk in the argument list.

Another alternative would be to add a Stimulus keyword that specifies the name of the first argument that should be treated as keyword-only (it would be k here), but this could cause problems if a higher level interface decides to re-order arguments.

I like the star notation. Can you please propose something, @ntamas?

I'll come up with a patch in Stimulus and we'll see how the idea works out.

See the Stimulus revision tagged with 0.21.0.

There are now three alternative ways to indicate keyword-only arguments:

You can add KW in front of the type of the argument in the PARAMS key to mark an argument as keyword-only.

You can add * in the middle of the argument list at the point where the keyword-only arguments start.

You can add a key named FIRST_KW_PARAM next to PARAMS and it must contain the name of the first keyword-only argument. This is primarily meant for backward compatibility with Stimulus 0.20 and earlier as the other two options would fail on earlier versions of Stimulus.

I tried to update the RR code generator to respect keyword-only arguments, but I haven't tested it. If it does not work, look in RRCodeGenerator, this is where the magic happens.

Next up would be to update functions.yaml in the C core to indicate which arguments should be keyword-only in generated interfaces.

If you want to make k a mandatory keyword argument, I would rather specify this as a custom parameter list for the R interface, and also give a default of k=1. In this specific case, I wouldn't mark it as mandatory keyword in the C core. Shall I go ahead?

While this feels natural in R, it wouldn't be so in Mathematica. Not sure about Python. Note that when using this function, people would always want to specify k.

@ntamas How does the * work with parameter reordering? Is it allowed to be used in PARAM_ORDER? One difficulty is that in C, weights usually comes second, yet it should probably always be keyword-only in R.

Is it allowed to be used in PARAM_ORDER?

No, only in PARAMS. It's a neat idea, though. I'll see what can be done.

How does the * work with parameter reordering?

In the parsed parameter specification, each parameter has a property named is_keyword_only that specifies whether the parameter is to be used as a keyword argument only if the language supports keyword arguments. The * is essentially a shortcut for specifying this for an argument and all subsequent arguments automatically. This comes before the parameter reordering.

Note that in theory, one could end up in a situation where keyword-only and non-keyword-only arguments are intermixed in the parameter ordering. It is up to the language-specific code generator to deal with such a situation. The RR code generator collects non-keyword-only args first and keyword-only args second (but respects PARAM_ORDER within each sublist). The Python code generator throws an error because it was easier to do it that way.

PARAM_ORDER now supports * from Stimulus 0.21.2. FWIW it also supports ..., which means "all the parameters that have not been listed explicitly so far, in their original order".

krlmlr · 2023-12-29T21:44:26Z

I'm massaging the .yaml in other PRs, there will be more conflicts.

krlmlr · 2023-12-29T22:26:16Z

After #1043, there should be no more conflicts for now.

szhorvat · 2023-12-30T01:29:42Z

What would a test look like here?

Yes, I will add tests.

szhorvat · 2023-12-30T11:01:33Z

A good test would be:

g <- make_ring(5)
res <- k_shortest_paths(g, 1, 2, k = 3)

@krlmlr How do I check that res has the expected value (since it's just just a number, but a more complicated expression)?

szhorvat · 2023-12-30T13:15:58Z

Conflicts resolved.

krlmlr · 2023-12-30T16:07:28Z

I would do something like:

expect_length(res$vpaths, 2)
expect_equal(as.numeric(res$vpaths[[1]]), c(1, 2))
expect_equal(as.numeric(res$vpaths[[2]]), c(1, 5, 4, 3, 2))
...

krlmlr

Very nice, thanks!

krlmlr · 2023-12-30T21:23:52Z

tools/stimulus/functions-R.yaml

      PP: get.all.simple.paths.pp

 igraph_get_k_shortest_paths:
+    PARAM_ORDER: graph, from, to, *, k, ...


I wonder why we need to override this here. No strong opinion.

I did this to be able to make weights optional. This function will often be used on unweighted graphs. Is there a better way to do this?

It's fine, but is this order something to be considered upstream too? To me, this order seems reasonable for other interfaces too.

Well, yes, but probably too late for that. If we ever reorder arguments in all functions in the C core, this would make sense, but it's unlikely that'll happen. When we settled on the argument ordering, we forgot about the consequences for auto-generation. CC @ntamas

https://github.com/igraph/igraph/wiki/Guidelines-for-function-argument-ordering

Interesting.

In R land, we have the attribute. R objects are immutable, it's easy to remove weights, or to set new weights, without changing the graph object. I wonder if we even need the weights argument.

szhorvat requested review from krlmlr and maelle December 28, 2023 22:29

szhorvat force-pushed the feat/k_shortest_paths branch from f8db3ad to afe79f9 Compare December 28, 2023 22:44

krlmlr mentioned this pull request Dec 29, 2023

refactor: Replace loop by lapply() when returning vertex and edge sets #1033

Merged

krlmlr reviewed Dec 29, 2023

View reviewed changes

krlmlr mentioned this pull request Dec 29, 2023

Add ellipsis #1045

Open

szhorvat force-pushed the feat/k_shortest_paths branch 2 times, most recently from 45565fb to 59d7b03 Compare December 30, 2023 13:07

szhorvat force-pushed the feat/k_shortest_paths branch 3 times, most recently from 4b05202 to 9b034b3 Compare December 30, 2023 20:59

szhorvat marked this pull request as ready for review December 30, 2023 20:59

szhorvat requested a review from krlmlr December 30, 2023 20:59

krlmlr changed the title ~~feat: k_shortest_paths()~~ feat: New k_shortest_paths() to compute the first k shortest paths between two vertices Dec 30, 2023

krlmlr approved these changes Dec 30, 2023

View reviewed changes

krlmlr added the mergequeue label Dec 30, 2023

szhorvat added 2 commits December 30, 2023 21:24

import check_dots_empty()

508fb80

feat: k_shortest_paths()

fc61c8c

tests: k_shortest_paths()

58377eb

aviator-app bot force-pushed the feat/k_shortest_paths branch from 9b034b3 to 58377eb Compare December 30, 2023 21:24

szhorvat mentioned this pull request Dec 30, 2023

Avoid .Call() in code other than aaa-auto.R #1058

Open

aviator-app bot merged commit dd51c56 into main Dec 30, 2023

aviator-app bot deleted the feat/k_shortest_paths branch December 30, 2023 22:03

github-actions bot locked as resolved and limited conversation to collaborators Dec 30, 2024

		@@ -819,14 +819,14 @@ get_k_shortest_paths_impl <- function(graph, weights=NULL, k, from, to, mode=c("
		res <- .Call(R_igraph_get_k_shortest_paths, graph, weights, k, from-1, to-1, mode)

	get_k_shortest_paths_impl <- function(graph, from, to, k, weights=NULL, mode=c("out", "in", "all", "total")) {
	get_k_shortest_paths_impl <- function(graph, from, to, ..., k, weights=NULL, mode=c("out", "in", "all", "total")) {
	check_dots_empty()

Uh oh!

Conversation

szhorvat commented Dec 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aviator-app bot commented Dec 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current Aviator status

Uh oh!

szhorvat commented Dec 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

szhorvat commented Dec 28, 2023

Uh oh!

szhorvat commented Dec 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

szhorvat commented Dec 28, 2023

Uh oh!

ntamas commented Dec 28, 2023

Uh oh!

krlmlr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szhorvat Dec 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krlmlr commented Dec 29, 2023

Uh oh!

krlmlr commented Dec 29, 2023

Uh oh!

szhorvat commented Dec 30, 2023

Uh oh!

szhorvat commented Dec 30, 2023

Uh oh!

szhorvat commented Dec 30, 2023

Uh oh!

krlmlr commented Dec 30, 2023

Uh oh!

krlmlr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szhorvat Dec 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

szhorvat commented Dec 28, 2023 •

edited

Loading

aviator-app bot commented Dec 28, 2023 •

edited

Loading

szhorvat commented Dec 28, 2023 •

edited

Loading

szhorvat commented Dec 28, 2023 •

edited

Loading

szhorvat Dec 30, 2023 •

edited

Loading

szhorvat Dec 30, 2023 •

edited

Loading