feat!: Allow change of attribute type when setting attribute for all vertices or edges, only attributes of length 1 or the length of the target set allowed

[krlmlr]

Closes #466.

This needs a full revdepcheck in all cases, and a cleanup (decide on ATTRIBUTE_STRICT_RECYCLING).

[krlmlr]

I initiated a revdepcheck with ATTRIBUTE_STRICT_RECYCLING <- TRUE .

[codecov]

Codecov Report

Merging #633 (e1a8366) into main (890a456) will increase coverage by 0.00%.
The diff coverage is 78.94%.

@@           Coverage Diff           @@
##             main     #633   +/-   ##
=======================================
  Coverage   62.69%   62.70%           
=======================================
  Files          73       73           
  Lines       21536    21571   +35     
=======================================
+ Hits        13502    13526   +24     
- Misses       8034     8045   +11     

Impacted Files	Coverage Δ
R/conversion.R	73.91% <ø> (ø)
R/structural.properties.R	86.35% <ø> (ø)
R/attributes.R	78.00% <75.00%> (-1.73%)	⬇️
R/interface.R	83.58% <100.00%> (+0.24%)	⬆️
R/iterators.R	91.68% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[krlmlr]

revdepcheck results are slightly devastating. Need to take a closer look.

[szhorvat]

revdepcheck results are slightly devastating.

Only 10/755 = 1.3% failed :-) Devastating is a bit of an overstatement, no?

[krlmlr]

@tilltnet @damianobaldan: I believe the failures in egor and riverconn are false positives, see https://github.com/igraph/rigraph/pull/633/files#diff-bbdda7f28391ee0ec001638e5603609582124345e0d65ebd4747cc592f9bd56d .

• egor: creating an empty graph with graph.data.frame() now creates attributes for all columns, instead of leaving them unset. The test is too strict, if you change it to assert that the length of the attribute is zero, it will work for both old and new igraph versions
• riverconn: the example doesn't make sense to me, it checks the non-existing attribute E(g)$id_barrier right after the graph is created

Can you please checkout this branch, run make , then R CMD INSTALL . , and then run your tests/examples to confirm?

[krlmlr]

Checking again the 19 failing packages in strict mode.

[krlmlr]

@zpneal @martirm @DominikRafacz @gemtc @gertvv @GrossSBM @MarcOhlmann @jefferis @guido-s @grunwaldlab @damianobaldan @schochastics: The current version of this PR implements stricter rules for assigning attributes to vertices and edges. To my knowledge, you are maintaining a package that fails to pass its checks with these new rules.

With the current CRAN version, it's possible to assign a vector of length 2 as a vertex attribute to a graph with 10 vertices, via recycling. Similarly, for edges. This also means that you can assign a vector of length 3 as a vertex attribute to the same graph, which is most likely an error. See the following example.

library(igraph, warn.conflicts = FALSE)

g <- make_ring(10)

# works
V(g)$foo <- 1
V(g)$foo <- 1:10
E(g)$foo <- 1
E(g)$foo <- 1:10

# may be a mistake
V(g)$bar <- 1:2
V(g)$bar <- 1:3
#> Warning in value_in[seq_along(value_in)] <- value: number of items to replace is
#> not a multiple of replacement length
E(g)$bar <- 1:2
E(g)$bar <- 1:3
#> Warning in value_in[seq_along(value_in)] <- value: number of items to replace is
#> not a multiple of replacement length

^{Created on 2023-01-16 with reprex v2.0.2}

The new rules require that attribute vectors have length 1 or the same length as the number of vertices or edges. I wonder if there are genuine use cases for this recycling of attributes, or if the new strict rules have uncovered a hidden problem in your code. See the below example for symptoms.

library(igraph, warn.conflicts = FALSE)

g <- make_ring(10)

# works
V(g)$foo <- 1
V(g)$foo <- 1:10
E(g)$foo <- 1
E(g)$foo <- 1:10

# fails
V(g)$bar <- 1:2
#> Error in i_set_vertex_attr(x, attr(value, "name"), index = value, value = attr(value, : strict recycling (1)
V(g)$bar <- 1:3
#> Error in i_set_vertex_attr(x, attr(value, "name"), index = value, value = attr(value, : strict recycling (1)
E(g)$bar <- 1:2
#> Error in i_set_edge_attr(x, attr(value, "name"), index = value, value = attr(value, : strict recycling (2)
E(g)$bar <- 1:3
#> Error in i_set_edge_attr(x, attr(value, "name"), index = value, value = attr(value, : strict recycling (2)

^{Created on 2023-01-16 with reprex v2.0.2}

Can you please run devtools::install_github("igraph/rigraph@b99ff321") or pak::pak("igraph/rigraph@b99ff321") and let me know if your package makes legitimate use of attribute recycling? Note that you can always use rep(...) to create a vector of the desired length, this is likely to be safer than relying on automatic recycling.

[krlmlr]

See https://github.com/igraph/rigraph/pull/633/files#diff-bbdda7f28391ee0ec001638e5603609582124345e0d65ebd4747cc592f9bd56d for a description of the problems we found, you may need to click "load diff".

Packages not found on GitHub: Diderot, mstknnclust.

[schochastics]

This seems like a sensible choice. I fixed my example error which was not a legitimate use of recycling but an actual copy and paste issue schochastics/signnet@8ad0ea7

[damianobaldan]

thanks, fixed, there was an error in the example. Damiano Il giorno dom 15 gen 2023 alle ore 21:15 Kirill Müller < ***@***.***> ha scritto:

…

@tilltnet <https://github.com/tilltnet> @damianobaldan <https://github.com/damianobaldan>: I believe the failures in egor and riverconn are false positives, see https://github.com/igraph/rigraph/pull/633/files#diff-bbdda7f28391ee0ec001638e5603609582124345e0d65ebd4747cc592f9bd56d . - egor: creating an empty graph with graph.data.frame() now creates attributes for all columns, instead of leaving them unset. The test is too strict, if you change it to assert that the length of the attribute is zero, it will work for both old and new igraph versions - riverconn: the example doesn't make sense to me, it checks the non-existing attribute E(g)$id_barrier right after the graph is created Can you please checkout this branch, run make , then R CMD INSTALL . , and then run your tests/examples to confirm? — Reply to this email directly, view it on GitHub <#633 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AW55PN57AM2XGTOWBJ4AY3TWSRLFLANCNFSM6AAAAAAT3YQDS4> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[zpneal]

Running devtools::install_github("igraph/rigraph@b99ff321") fails for me because make: /opt/R/arm64/bin/gfortran: No such file or directory. This seems to be a known issue for Mac M1 that doesn't have a simple solution (see https://stackoverflow.com/questions/70638118/configuring-compilers-on-mac-m1-big-sur-monterey-for-rcpp-and-other-tools). I'll try to correct the issue in backbone and incidentally packages, but may not be able to test them until the new igraph binary is available.

[ntamas]

You might need to install the experimental gfortran fork from https://mac.r-project.org/tools/ .

Alternatively, use gfortran from Homebrew -- that's what I do on my M1 Mac. I use it in tandem with ccache with the following ~/.R/Makevars file and never had any problems with it:

VER=
CCACHE=ccache

CC=$(CCACHE) clang$(VER)
CXX=$(CCACHE) clang++$(VER)
CXX11=$(CCACHE) clang++$(VER)
CXX14=$(CCACHE) clang++$(VER)
FC=$(CCACHE) gfortran$(VER)
F77=$(CCACHE) gfortran$(VER)

CFLAGS += -O0
CXXFLAGS += -O0

(Note that I'm adding -O0 to the compiler flags to ease debugging; you might not want this).

[zpneal]

Thanks @ntamas for the link. I tried installing experimental gfortran, which seemed to work. But, now running devtools::install_github("igraph/rigraph@b99ff321") fails with

ld: warning: directory not found for option '-L/opt/R/arm64/gfortran/lib/gcc/aarch64-apple-darwin20.2.0/11.0.0'
ld: library not found for -lemutls_w
clang: error: linker command failed with exit code 1 (use -v to see invocation)

Apologies - I have limited experience with compilers and working in the Mac terminal.

[zpneal]

@krlmlr I've fixed the error in backbone. I was assigning attributes to only one vertex set in a bipartite graph. Because the attributes assigned to the other vertex set didn't matter, I was just allowing the attribute vector to recycle. But, I now explicitly set them.

The error in incidentally is unrelated to igraph.

[jefferis]

Thanks @krlmlr! In my case the error now happens because it is no longer possible to add a null attribute (I have code which either adds the correct number of edge attributes or none by setting NULL). Obviously it's not the end of the world to have to check if attributes are empty and adjust the call accordingly but I wanted to check that this was a conscious design decision?

Used to work with igraph 1.3.5:

g=make_empty_graph(n=2)
g2=add.edges(g, cbind(a=1, b=2), segid=NULL)

Now errors with this branch:

> g=make_empty_graph(n=2)
> g2=add.edges(g, cbind(a=1, b=2), segid=NULL)
Error in i_set_edge_attr(graph = graph, name = name, index = index, value = value) : 
  strict recycling (2)

Now I would have to do:

g2<-if(is.null(segids))
  add.edges(g, cbind(a=1, b=2))
else
  add.edges(g, cbind(a=1, b=2), segid=segids)

obviously if there are multiple attributes to add it gets more complicated and I end up having to do do.call etc.

[krlmlr]

Thanks, this is very helpful. We certainly can leave such NULL attributes untouched. I'll update this PR and follow up, no need to act yet.

[krlmlr]

Restarted revdepchecks for broken packages.

[jefferis]

@krlmlr thanks for the change in e89ab9e. Now all but one of my tests pass. The last one was actually testing for exactly this kind of attribute length issue, so I guess I would need to change the test

  expect_warning(testg <- as.ngraph(testd, graph.attributes = gatts, 
                               vertex.attributes=list(X=testd$X[-1])))

to something like:

  # nb previously attribute lengths gave a mismatch
  if(igraph::igraph_version()>=numeric_version("1.3.5.9098"))
    expect_error(testg <- as.ngraph(testd, graph.attributes = gatts, 
                               vertex.attributes=list(X=testd$X[-1])))
  else
    expect_warning(testg <- as.ngraph(testd, graph.attributes = gatts, 
                                    vertex.attributes=list(X=testd$X[-1])))

thoughts?

[krlmlr]

The mstknnclust error seems to come from here:

https://github.com/cran/mstknnclust/blob/master/vignettes/guide.Rmd#L104

We need to notify the maintainer via e-mail.

Other than that, are we okay with this breaking change?

[ntamas]

Yes I am.

[krlmlr]

@DominikRafacz @tilltnet @Demiperimetre @guido-s @jmanitz @grunwaldlab: This breaks checks in packages you maintain, see #633 (comment) for details. If you think this is incorrect behavior on our end, please comment here (while this PR is open) or file a new issue.

[krlmlr]

New error message format:

Length of new attribute value must be 1 or 12, the number of target edges, not 6

[ntamas]

LGTM. Shall I merge the revdepcheck results into main as well or will they have to be removed from the PR?

[jefferis]

@krlmlr I've just run into a new error whose source I haven't tracked down. The gist of it is that I was replacing a subset of the weight attributes on edges of a graph (defined by a single path across the graph) with 0. The result is that with igraph from this PR I end up with the new graph with weight attributes having the length of the subset I just changed ie all the other edges lose their weights.

[jefferis]

@krlmlr the bug is almost certainly introduced in this PR as remotes::install_github('igraph/rigraph@191b3e65160c49cd9aa176b28dc445a7d0c45579') which seems to be the commit is fine. So please don't merge yet. I will try to produce a reprex for you.

[jefferis]

@krlmlr with remotes::install_github('igraph/rigraph@191b3e6516'):

> set.seed(42)
> g <- make_ring(10)
> E(g)$weight=sample(1:3, size = length(E(g)), replace = T)
> E(g)$weight
 [1] 1 1 1 1 2 2 2 1 3 3
> distances(g,v=1, to=6)
     [,1]
[1,]    6
> p=shortest_paths(g, from=1, to = 6)$vpath[[1]]
> E(g, path=p)$weight
[1] 1 1 1 1 2
> E(g, path=p)$weight=0
> E(g, path=p)$weight
[1] 0 0 0 0 0
> E(g)$weight
 [1] 0 0 0 0 0 2 2 1 3 3
> distances(g,v=1, to=2:10)
     [,1] [,2] [,3] [,4] [,5] [,6] [,7] [,8] [,9]
[1,]    0    0    0    0    0    2    4    5    3
> distances(g,v=1, to=6)
     [,1]
[1,]    0

with remotes::install_github('igraph/rigraph@882eeaa85f') (current last commit of PR):

> set.seed(42)
> g <- make_ring(10)
> E(g)$weight=sample(1:3, size = length(E(g)), replace = T)
> E(g)$weight
 [1] 1 1 1 1 2 2 2 1 3 3
> distances(g,v=1, to=6)
     [,1]
[1,]    6
> 
> p=shortest_paths(g, from=1, to = 6)$vpath[[1]]
> E(g, path=p)$weight
 [1] 1 1 1 1 2 2 2 1 3 3
> E(g, path=p)$weight=0
> E(g, path=p)$weight
[1] 0 0 0 0 0
> E(g)$weight
[1] 0 0 0 0 0
> distances(g,v=1, to=2:10)
Error in distances(g, v = 1, to = 2:10) : 
  At core/paths/dijkstra.c:116 : Weight vector length does not match, Invalid value

Notice how E(g)$weight goes from length 10 to length 5 after the manipulation using the codebase from the PR.

> E(g)$weight
 [1] 1 1 1 1 2 2 2 1 3 3

vs

> E(g)$weight
[1] 0 0 0 0 0

Somewhere the partial replacement is being used to replace the complete set of edge attributes.

[jefferis]

The bug is introduced by e0536d1 and I assume that there is a logic error here:

    complete <- is_complete_iterator(index)

[snip]

    if (complete) {
      eattrs[[name]] <- value_in
    } else {
      eattrs[[name]][index] <- value_in
    }

I don't know what is_complete_iterator is supposed to signify, but it doesn't mean that the indices refer to the whole graph since in my example:

Browse[2]> index
+ 5/10 edges from 66a55e4:
[1] 1--2 2--3 3--4 4--5 5--6
Browse[2]> is_complete_iterator(index)
[1] TRUE

i.e. a path is considered complete when is_all==TRUE

Browse[2]> is_complete_iterator
function (x) 
{
    identical(attr(x, "is_all"), TRUE)
}
<bytecode: 0x7fdbfd458cf8>
<environment: namespace:igraph>
Browse[2]> index
+ 5/10 edges from 66a55e4:
[1] 1--2 2--3 3--4 4--5 5--6
Browse[2]> attributes(index)
$class
[1] "igraph.es"

$is_all
[1] TRUE

$env
<weak reference>

$graph
[1] "66a55e4c-95a2-44a1-869a-d436a9e0b548"

$name
[1] "weight"

$value
[1] 0

I hope this helps suggest an appropriate fix @krlmlr. Thanks to the whole team for your continued work on this vital and valuable package!

[krlmlr]

Thanks, on it.

[krlmlr]

Fixed, restarted revdepchecks for the broken packages.

I propose to keep revdep results in version control.

[krlmlr]

@wael-sadek: Can you please take a look why the fledge workflow has run for this PR?

[krlmlr]

@DominikRafacz @tilltnet @Demiperimetre @guido-s @jmanitz @grunwaldlab: This still breaks checks in packages you maintain, see #633 (comment) for details. If you think this is incorrect behavior on our end, please comment here (while this PR is open) or file a new issue. Thanks!

[jorgeklz]

You can find mstknn package here https://github.com/cran/mstknnclust

[ntamas]

@krlmlr Is this ready to be merged back into main now? We should get a release out soon to fix the warnings on CRAN