rigraph icon indicating copy to clipboard operation
rigraph copied to clipboard

Warning

Open maelle opened this issue 3 months ago • 15 comments

From #2058 when running test-centrality we get

> G <- sample_gnm(10, sample(1:20, 1))
+     hs <- hits_scores(G)
Warning message:
In hub_and_authority_scores_impl(graph = graph, weights = weights,  :
  At vendor/cigraph/src/centrality/hub_authority.c:294 : Hub and authority scores requested for undirected graph. These are the same as eigenvector centralities.

How should we handle this warning? Does this mean hits_score() should run is.directed() on its input before calling another function?

@schochastics @szhorvat

maelle avatar Oct 09 '25 11:10 maelle

yes I think that check makes sense and then we need to display a proper R level warning?

schochastics avatar Oct 09 '25 14:10 schochastics

Does this mean hits_score() should run is.directed() on its input before calling another function?

No. The C core handles the warning so you don't have to.

It simply makes no sense to use this function with undirected graph, even though people naively try to. We added this warning to inform the user.

szhorvat avatar Oct 09 '25 22:10 szhorvat

But I would still vote for an R level warning? At vendor/cigraph/src/centrality/hub_authority.c:294 : looks cryptic to some users?

schochastics avatar Oct 10 '25 07:10 schochastics

It is not pretty, but this is how the system works and how all errors and warnings are displayed, with a reference to the location in the source. There are many places where warnings are issued. Wouldn't it be too much work to override all of them from R? Also, if the warning text is improved in C, the improvement wouldn't be picked up in R.

Note that printing the source location is a choice: The R/igraph error/warning function has code to include this information. You could remove if if you don't want it. But I strongly recommend against removing, as sometimes it's much easier to identify what went wrong if we have a reference to the sources.

However, I would recommend removing vendor/cigraph or even vendor/cigraph/src if we can make that work, as there's no useful information in that. The rest of the path is perfectly sufficient.

szhorvat avatar Oct 10 '25 09:10 szhorvat

I have absolutely ni strong feelings about this and we can leave it as is. I just thought I had never seen these c level errors in R before

schochastics avatar Oct 10 '25 18:10 schochastics

I thought I could redirect the call to eigen_centrality() but its arguments are not the same as hits_score().

maelle avatar Oct 14 '25 12:10 maelle

I'm less and less a fan of the stress test:

How could we improve that test?

library("igraph")
#> 
#> Attaching package: 'igraph'
#> The following objects are masked from 'package:stats':
#> 
#>     decompose, spectrum
#> The following object is masked from 'package:base':
#> 
#>     union
G <- sample_gnm(10, sample(1:20, 1), directed = TRUE)
hs <- hits_scores(G)
#> Warning in hub_and_authority_scores_impl(graph = graph, weights = weights, : At
#> vendor/cigraph/src/centrality/hub_authority.c:77 : More than 30% of hub or
#> authority scores are zeros. The presence of zero values indicates that the
#> solution is not unique, thus the returned result may not be meaningful.

Created on 2025-10-14 with reprex v2.1.1

maelle avatar Oct 14 '25 13:10 maelle

https://github.com/igraph/rigraph/blob/ba2011f37d5d15d827a3b890a7bcf04b3f1b7940/tests/testthat/test-centrality.R#L144

maelle avatar Oct 14 '25 13:10 maelle

The warning isn't accurate? @szhorvat

el <- matrix(
  c(
    1, 1, 1, 3, 3, 3, 4, 4, 5, 6, 6, 7, 7, 7, 7, 8, 8, 8, 9, 9, 10, 5, 7, 8, 10,
    8, 9, 10, 6, 9, 4, 7, 1, 6, 10, 8, 6, 7, 10, 2, 7, 1
  ),
  nrow = 21L,
  ncol = 2L
)
g <- graph_from_edgelist(el, directed = TRUE)
hs <- hits_scores(g)
#> Warning in hub_and_authority_scores_impl(graph = graph, weights = weights, : At
#> vendor/cigraph/src/centrality/hub_authority.c:81 : More than 30% of hub or
#> authority scores are zeros. The presence of zero values indicates that the
#> solution is not unique, thus the returned result may not be meaningful.
hub <- hs$hub_vector
auth <- hs$authority_vector

hub
#>  [1] 0.50132341 0.00000000 0.68439809 0.62821626 0.08653582 0.22495909
#>  [7] 1.00000000 0.82792414 0.22495909 0.12644077
auth
#>  [1] 0.35867759 0.07163074 0.00000000 0.07163074 0.15962976 0.78207620
#>  [7] 0.56651614 0.69597030 0.24547826 1.00000000
sum(hub==0)/length(hub)
#> [1] 0.1
sum(auth==0)/length(auth)
#> [1] 0.1

Created on 2025-10-14 with reprex v2.1.1

schochastics avatar Oct 14 '25 13:10 schochastics

Independent of this, I think the random graphs used are too sparse and disconnected

This might be too conservative but this circumvents the c level warnings

for (i in 1:100) {
  G <- sample_gnm(10, sample(15:25, 1), directed = FALSE)
  while (!is_connected(G)) {
    G <- sample_gnm(10, sample(15:25, 1), directed = FALSE)
  }
  G <- as_directed(G, mode = "mutual")
  a <- hits_scores(G)
}

schochastics avatar Oct 14 '25 13:10 schochastics

I thought I could redirect the call to eigen_centrality() but its arguments are not the same as hits_score().

I suggest NOT to try to override the warning, unless you are prepared to adapt every time the warning is changed (or the warning text is updated/polished) in the C core.

Independent of this, I think the random graphs used are too sparse and disconnected

Yes.

The warning isn't accurate? @szhorvat

Can you please open an issue in the C core repo, and copy this comment there?

szhorvat avatar Oct 14 '25 16:10 szhorvat

tracking: https://github.com/igraph/igraph/issues/2872

schochastics avatar Oct 15 '25 08:10 schochastics

@schochastics would there be a way for us to re-format all C level warnings?

E.g. if we get

 At vendor/cigraph/src/centrality/hub_authority.c:294 : Hub and authority scores requested for undirected graph. These are the same as eigenvector centralities.

We'd display

x Hub and authority scores requested for undirected graph. 
These are the same as eigenvector centralities.
Source: centrality/hub_authority.c:294

So, splitting the text into sentences, and putting the line indication last (useful for bug reports but weird for the R user)

How could we catch and modify these warnings?

@szhorvat note that with this method we'd get the updated warnings.

maelle avatar Oct 17 '25 12:10 maelle

@maelle: Absolutely. Instead of calling Rf_warning(), we would call into an R function we provide that forwards to cli::cli_warn() .

@szhorvat: Do you remember why R_igraph_warning_handler() only records the warning instead of emitting it? Do you see any reason why that recording shouldn't happen at the R level? In this case, we'd also change R_igraph_warning_handler() to record in R.

krlmlr avatar Oct 19 '25 02:10 krlmlr

The only reason not to emit the warning immediately is if Rf_warning() can potentially longjump instead of returning, preventing igraph from freeing allocated resources. Can R be configured to immediately abort on a warning? If not, the better way is to emit the warning immediately.

The errors are collected so that the abort could be delayed until all igraph resources have been freed.

szhorvat avatar Oct 19 '25 09:10 szhorvat