igraph / rigraph

igraph R package
https://r.igraph.org
544 stars 200 forks source link

Default max/min option value is different for cliques() and max_cliques() #896

Open szhorvat opened 1 year ago

szhorvat commented 1 year ago

The default max/min option value is different for cliques() and max_cliques():

cliques(graph, min = 0, max = 0)
max_cliques(graph, min = NULL, max = NULL, subset = NULL, file = NULL)

Here both 0 and NULL indicate: "do not set a bound on the clique size". We should settle on using either 0 or NULL consistently throughout all these functions.

Other affected functions: count_max_cliques, clique_size_counts, weighted_cliques, ...?

CC @krlmlr @maelle

maelle commented 1 year ago

Why not make the default value -Inf and Inf then?

szhorvat commented 1 year ago

Something like that would be nice and intuitive, though -Inf doesn't make sense as the smallest possible clique size is 1. I would go with 0 and Inf (instead of 1 and Inf), for the simple reason that these values are meaningful both for the weighted and non-weighted clique problem.


On the C side, there are special values that indicate "no constraint". For the clique functions these are 0 or negative, i.e. invalid clique sizes. The simple solution is to use the same in R, as this makes it easy to auto-generate the interface. But the R interface could be nicer if we used more intuitive values (note that Inf is not possible in C). Thus, to do this, we must either use a hand-written wrapper or improve the interface generator so it can translate values appropriately.

@ntamas Is the interface generator already capable of translating special values just for certain arguments? If not, this would be nice to implement.

szhorvat commented 1 year ago

This is not a documentation issue, that label doesn't fit here.

maelle commented 1 year ago

R has an is.infinite() function that could be used before sending stuff over to C I guess?

szhorvat commented 1 year ago

Of course it's solvable. The concern is that a similar situation exists with many functions, not just clique functions. Ideally we want to allow the R wrapper to be auto-generated instead of coding it manually, to save time. This will require changes to stimulus, the interface generator.

ntamas commented 1 year ago

Is the interface generator already capable of translating special values just for certain arguments? If not, this would be nice to implement.

Not yet, but it's easy to add. Right now the defaults come from types.yaml, which is always extended on a per-interface basis to add conversion functions. We can probably add a DEFAULT keyword for function definitions in functions.yaml, which could then be used to override the special values on a per-function basis.

ntamas commented 1 year ago

Is the interface generator already capable of translating special values just for certain arguments? If not, this would be nice to implement.

This is now possible from stimulus 0.18.0 onwards. The correct syntax in functions.yaml is as follows:

igraph_some_function:
    DEFAULT:
        param_name: new_default_value