Union/intersection of graphs with vertex names

[iosonofabio]

Trying to address #82 and #269

For now there is a stub of union and one for intersection. Much of the logic (but not all) is in place, details and tests missing.

The obvious question from my side at this stage is about architecture:

1. Is the file operators.py an ok place for this?
2. Is it ok to have these two global functions within the igraph namespace and then hook a call to them in (to be defined) instance methods of the Graph class (i.e. to-be Graph.union, Graph.intersection)?
3. If 2 is fine, then we can reroute the __and__ and __or__ operators into this code path

Thanks!
Fabio

[iosonofabio]

I now drafted a change in the Graph.union interface that recovers edgemaps from the C layer, just like in R. That way we should be able to keep edge attributes during union/intersection.

Because I'm a little rusty in Python C API calls, this might take a few iterations. Any comment on the interface would be welcome.

[iosonofabio]

This is ready for review now. The changes reach quite a bit deeper than initially suspected. Summary:

• operators.py implements union and intersection of graphs like in the R interface - merging by name and keeping attributes
• __init__.py has changes in the Graph class to use those functions
• operators.c/h include modified interface functions dealing with edgemaps
• graphobject.c/h lost the original interfaces instead
• more tests are added to make sure the new functions work as expected

In addition, one trick was necessary, which is to add a modified conversion function that fills a vector of pointers to graphs and also stores the python type/class of the initial pointer (graph). That seems necessary to ensure the resulting union/intersection graph has the properties vs and es when generated at the C API interface.

One difference from R is that here the code is a little verbose but less obscure, whereas R uses a utility function to deal with attributes that shortens the code but makes it significantly harder to read.

[iosonofabio]

Alright, I wrote the disjoint union and then lost it in a git stash... so I ended up recoding it.

Bottomline is now it's there and tested. Please have a look whenever you have a minute and we should be close to merging.

[ntamas]

Thanks! This is a larger chunk so it will take me a while, stay tuned.

[iosonofabio]

@ntamas I realize you are busy with other stuff, so if you would like me to do anything on this PR please let me know

[ntamas]

I finally managed to review your changes -- sorry it took such long time.

I'm mostly okay with the changes, but I have found a few Python reference leaks in the newly created _disjoint_union(), _union() and _intersection() functions that should definitely be fixed.

As for the attribute logic in operators.py, I feel it's okay and we are probably covered by the test cases, but I found the code hard to read due to the usage of many two- or three-letter variable names. If this PR is to be merged, I will probably go through these functions with a refactor tool and rename the variables to spell out their purpose explicitly.

[iosonofabio]

Fantastic thank you for the thorough review!

I've fixed the things you mentioned. I'm in favour of refactoring if that helps. I've tried to rename the worst offenders, but there might be a few more that you still find confusing, so just go ahead and rename them into whatever seems to make sense.

Waiting for CI as I write.

[iosonofabio]

I cleaned up the tests a little, including unrelated things that are minor. I know I should have done it in a separate PR and then rebase, but that seemed like a huge effort for such small things.

The takeaway is that the tests fail on my machine on a single thing now, which has to do with igraph/igraph#597. That issue says you were going to rewire python-igraph to the fixed C core, has that happened already?

Thanks!

[iosonofabio]

Well seems like that, since CI passes.

So it's done except for some optional refactoring of operators.py, at your discretion.

[iosonofabio]

Haha awesome, thank you!