Skip to content

fix: Fix direction of edges when restoring#805

Merged
krlmlr merged 4 commits intomainfrom
b-restore-edge-dir
May 21, 2023
Merged

fix: Fix direction of edges when restoring#805
krlmlr merged 4 commits intomainfrom
b-restore-edge-dir

Conversation

@krlmlr
Copy link
Copy Markdown
Contributor

@krlmlr krlmlr commented May 21, 2023

Should the order be from->to?

@krlmlr krlmlr requested a review from Antonov548 May 21, 2023 15:39
@krlmlr
Copy link
Copy Markdown
Contributor Author

krlmlr commented May 21, 2023

This appears to work also with igraph/igraphdata#5.

@krlmlr
Copy link
Copy Markdown
Contributor Author

krlmlr commented May 21, 2023

How can we test correct restoration in this repo?

@krlmlr
Copy link
Copy Markdown
Contributor Author

krlmlr commented May 21, 2023

On the current main branch:

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

g <- make_ring(3, directed = TRUE)
g
#> IGRAPH a6d007c D--- 3 3 -- Ring graph
#> + attr: name (g/c), mutual (g/l), circular (g/l)
#> + edges from a6d007c:
#> [1] 1->2 2->3 3->1

gs <- unserialize(serialize(g, NULL))
gs
#> IGRAPH a6d007c D--- 3 3 -- Ring graph
#> + attr: name (g/c), mutual (g/l), circular (g/l)
#> + edges from a6d007c:
#> [1] 2->1 3->2 1->3

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

@Antonov548
Copy link
Copy Markdown
Contributor

Antonov548 commented May 21, 2023

I also created small C example to play around with this in igraph. It should be from and to. Originally I was confused by the order in igraph_add_edges function.

#include <igraph.h>

void print_vector_int(igraph_vector_int_t *v) {
    igraph_integer_t i, n = igraph_vector_int_size(v);
    for (i = 0; i < n; i++) {
        printf(" %" IGRAPH_PRId, VECTOR(*v)[i]);
    }
    printf("\n");
}


int main(void) {

    igraph_t g;
    igraph_vector_int_t v1, v2;

    /* simple use */
    igraph_vector_int_init(&v1, 8);
    VECTOR(v1)[0] = 0;
    VECTOR(v1)[1] = 1;
    VECTOR(v1)[2] = 1;
    VECTOR(v1)[3] = 2;
    VECTOR(v1)[4] = 3;
    VECTOR(v1)[5] = 2;
    VECTOR(v1)[6] = 2;
    VECTOR(v1)[7] = 2;
    
    igraph_bool_t directed = 1;

    igraph_create(&g, &v1, 0, directed);

    igraph_vector_int_t v;
    igraph_integer_t i, s=igraph_vector_int_size(&g.from);
    igraph_vector_int_init(&v, s*2);

    for (i = 0; i < s; ++i) {
        igraph_integer_t from_v=VECTOR(g.from)[i];
        igraph_integer_t to_v=VECTOR(g.to)[i];
        
        igraph_vector_int_set(&v, i*2, from_v);
        igraph_vector_int_set(&v, i*2+1, to_v);
    }

    print_vector_int(&g.from);
    print_vector_int(&g.to);

    print_vector_int(&v);

    igraph_t g2;
    igraph_empty(&g2, g.n, directed);
    igraph_add_edges(&g2, &v, NULL);

    print_vector_int(&g2.from);
    print_vector_int(&g2.to);

    igraph_destroy(&g);
    igraph_destroy(&g2);
    igraph_vector_int_destroy(&v);

    return 0;
}

@krlmlr
Copy link
Copy Markdown
Contributor Author

krlmlr commented May 21, 2023

Working on a test here.

@krlmlr krlmlr mentioned this pull request May 21, 2023
@krlmlr
Copy link
Copy Markdown
Contributor Author

krlmlr commented May 21, 2023

@Antonov548: The test failures are interesting. Can you replicate on Ubuntu? What happens here?

https://github.com/igraph/rigraph/actions/runs/5038508208/jobs/9036118195?pr=805#step:6:205

@krlmlr
Copy link
Copy Markdown
Contributor Author

krlmlr commented May 21, 2023

I can't replicate on Ubuntu.

I'll remove the snapshot test for now and merge.

@krlmlr krlmlr changed the title Fix direction of edges when restoring fix: Fix direction of edges when restoring May 21, 2023
@krlmlr krlmlr merged commit bced088 into main May 21, 2023
@krlmlr krlmlr deleted the b-restore-edge-dir branch May 21, 2023 19:21
@Antonov548
Copy link
Copy Markdown
Contributor

I'll remove the snapshot test for now and merge.

It's very strange. But I can replicate it time to time. Sometime it prints edges some time not. I there any function which is responsible for this printing?

@krlmlr
Copy link
Copy Markdown
Contributor Author

krlmlr commented May 21, 2023

Yes, weird. We need a reprex first. Does it fail on your machine when you print in a loop? How many iterations do you need?

@krlmlr krlmlr mentioned this pull request May 21, 2023
@Antonov548
Copy link
Copy Markdown
Contributor

Yes, weird. We need a reprex first. Does it fail on your machine when you print in a loop? How many iterations do you need?

For me seems it failing when I don't watch how tests are running. I mean I just collapse VS code window 😕
I don't know if it really could be connected.
image

@krlmlr
Copy link
Copy Markdown
Contributor Author

krlmlr commented May 21, 2023

What version of R are you running? I'm not seeing failures with R 4.3.0 on Ubuntu.

@Antonov548
Copy link
Copy Markdown
Contributor

What version of R are you running? I'm not seeing failures with R 4.3.0 on Ubuntu.

I also have 4.3.0
image

@Antonov548
Copy link
Copy Markdown
Contributor

I see it coming from print.R file. And I see that printing of edges has some conditions. I will try to go over this code.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants