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

Unintended type conversions when using disjoint_union #761

Closed
bockthom opened this issue Apr 2, 2023 · 10 comments · Fixed by #1375
Closed

Unintended type conversions when using disjoint_union #761

bockthom opened this issue Apr 2, 2023 · 10 comments · Fixed by #1375
Assignees
Labels
bug an unexpected problem or unintended behavior
Milestone

Comments

@bockthom
Copy link

bockthom commented Apr 2, 2023

Bug Description
With the breaking change of igraph version 1.4.0, igraph now respects the concrete type of attributes when setting edge or vertex attributes (#633). While I am happy that the type is respect now, the function disjoint_union does not respect the attributes' types and does some weird type conversions.

How to Reproduce this Bug
Here is a minimum working example, in which the wrong behavior of disjoint_union can be seen:

# create two graphs
g1 <- make_graph(~ B -- C, C -- D)
g2 <- make_graph(~ A -- B, E -- D)

# add an edge attribute of class POSIXct to each graph
g1 <- set.edge.attribute(g1, "date", value = as.POSIXct(c("2021-01-01 01:01:01", "2022-02-02 02:02:02")))
g2 <- set.edge.attribute(g2, "date", value = as.POSIXct(c("2021-03-03 03:03:03", "2022-04-04 04:04:04")))

# create the union of the two graphs
u <- disjoint_union(g1,g2)

# now print the structure of g1, g2, and u
str(g1)
str(g2)
str(u)

You can find the corresponding output of the str(...) calls below – look for the date attribute in each of the three graphs to see that disjoint_union performs an unwanted type conversion: For g1 and g2, the class of the date attribute is POSIXct. However, for u, the class of the date attribute is num. This leads to various problems if you assume in your code that the dates are always given in POSIXct but then disjoint_union inadvertently converts them into numeric unix timestamps.

str(g1)
Class 'igraph'  hidden list of 10
 $ : num 3
 $ : logi FALSE
 $ : num [1:2] 1 2
 $ : num [1:2] 0 1
 $ : num [1:2] 0 1
 $ : num [1:2] 0 1
 $ : num [1:4] 0 0 1 2
 $ : num [1:4] 0 1 2 2
 $ :List of 4
  ..$ : num [1:3] 1 0 1
  ..$ : Named list()
  ..$ :List of 1
  .. ..$ name: chr [1:3] "B" "C" "D"
  ..$ :List of 1
  .. ..$ date: POSIXct[1:2], format: "2021-01-01 01:01:01" "2022-02-02 02:02:02"
 $ :<environment: 0x55f6dc22f0f8> 

str(g2)
Class 'igraph'  hidden list of 10
 $ : num 4
 $ : logi FALSE
 $ : num [1:2] 1 3
 $ : num [1:2] 0 2
 $ : num [1:2] 0 1
 $ : num [1:2] 0 1
 $ : num [1:5] 0 0 1 1 2
 $ : num [1:5] 0 1 1 2 2
 $ :List of 4
  ..$ : num [1:3] 1 0 1
  ..$ : Named list()
  ..$ :List of 1
  .. ..$ name: chr [1:4] "A" "B" "E" "D"
  ..$ :List of 1
  .. ..$ date: POSIXct[1:2], format: "2021-03-03 03:03:03" "2022-04-04 04:04:04"
 $ :<environment: 0x55f6e23ae7e8> 

str(u)
Class 'igraph'  hidden list of 10
 $ : num 7
 $ : logi FALSE
 $ : num [1:4] 1 2 4 6
 $ : num [1:4] 0 1 3 5
 $ : num [1:4] 0 1 2 3
 $ : num [1:4] 0 1 2 3
 $ : num [1:8] 0 0 1 2 2 3 3 4
 $ : num [1:8] 0 1 2 2 3 3 4 4
 $ :List of 4
  ..$ : num [1:3] 1 0 1
  ..$ : list()
  ..$ :List of 1
  .. ..$ name: chr [1:7] "B" "C" "D" "A" ...
  ..$ :List of 1
  .. ..$ date: num [1:4] 1.61e+09 1.64e+09 1.61e+09 1.65e+09
 $ :<environment: 0x55f6df448638> 

Version Information

Rest of the output of sessionInfo():

Matrix products: default
BLAS:   /usr/lib/x86_64-linux-gnu/atlas/libblas.so.3.10.3
LAPACK: /usr/lib/x86_64-linux-gnu/atlas/liblapack.so.3.10.3

locale:
 [1] LC_CTYPE=en_US.UTF-8       LC_NUMERIC=C              
 [3] LC_TIME=en_US.UTF-8        LC_COLLATE=en_US.UTF-8    
 [5] LC_MONETARY=en_US.UTF-8    LC_MESSAGES=en_US.UTF-8   
 [7] LC_PAPER=en_US.UTF-8       LC_NAME=C                 
 [9] LC_ADDRESS=C               LC_TELEPHONE=C            
[11] LC_MEASUREMENT=en_US.UTF-8 LC_IDENTIFICATION=C       

attached base packages:
[1] stats     graphics  grDevices utils     datasets  methods   base     

loaded via a namespace (and not attached):
[1] compiler_4.2.3 tools_4.2.3    packrat_0.9.1 

@hechtlC: FYI

@ntamas
Copy link
Member

ntamas commented Apr 7, 2023

Tracing shows that this line is to blame in the source of disjoint_union() (which implements the merging of the attributes entirely in R - the C disjoint union function does not deal with attributes):

attr[[exattr[a]]] <- c(attr[[exattr[a]]], ea[[exattr[a]]])

Shorter repro using reprex:

attr <- list()
attr$date <- c(attr$date, as.POSIXct("2021-01-01 01:01:01"))
attr$date
#> [1] 1609459261

Created on 2023-04-07 with reprex v2.0.2

@ntamas
Copy link
Member

ntamas commented Apr 7, 2023

I'm not fluent enough in R to decide what's the best way to deal with this so I'm leaving this up to @krlmlr

@krlmlr
Copy link
Contributor

krlmlr commented May 20, 2023

c() is broken, vctrs::vec_c() does a much better job. Do you feel strongly about this dependency?

Minimal reprex of the original problem:

options(conflicts.policy = list(warn = FALSE))
library(igraph)

# create two graphs
g1 <- make_graph(~ A - -B)
g2 <- make_graph(~ D - -E)

# add an edge attribute of class POSIXct to each graph
g1 <- set.edge.attribute(g1, "date", value = as.POSIXct(c("2021-01-01 01:01:01")))
g2 <- set.edge.attribute(g2, "date", value = as.POSIXct(c("2021-03-03 03:03:03")))

# create the union of the two graphs
u <- disjoint_union(g1, g2)

E(u)$date
#> [1] 1609459261 1614736983

Created on 2023-05-20 with reprex v2.0.2

@ntamas
Copy link
Member

ntamas commented May 20, 2023

Do you feel strongly about this dependency?

Feel free to add vctrs as a dependency if that's an easy fix. Seems like a pretty lightweight dependency for me.

@bockthom
Copy link
Author

Are there any news on this issue?
Do you already have a rough estimate about when this will be fixed?
Thanks in advance.

@krlmlr krlmlr added the bug an unexpected problem or unintended behavior label Jun 16, 2023
@krlmlr
Copy link
Contributor

krlmlr commented Jun 16, 2023

Thanks. We will prioritize issues next week.

@bockthom
Copy link
Author

Excuse me for asking again; several weeks have passed since the last comment. Can you estimate approximately when you will be able to fix this bug?

@krlmlr krlmlr added this to the triage milestone Feb 20, 2024
@bockthom
Copy link
Author

For your information: I checked this issue again, as I reported it when using igraph 1.4.0.
When using the most recent igraph version 2.0.3, unfortunately, this bug is still present.

@krlmlr
Copy link
Contributor

krlmlr commented Apr 13, 2024

We're now in a state where we can start tackling bugs like this. I can't give a timeline, though.

@maelle: I think using vctrs::vec_c() instead of c() in the appropritate place will fix this problem. I wonder if this problem occurs elsewhere.

@krlmlr
Copy link
Contributor

krlmlr commented May 23, 2024

Vignette section: https://vctrs.r-lib.org/articles/stability.html#c-and-vctrsvec_c .

Also useful: the rest of that vignette and https://vctrs.r-lib.org/articles/type-size.html .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug an unexpected problem or unintended behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants