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

Support DiHypergraph in from_bipartite_graph #633

Merged
merged 7 commits into from
Jan 18, 2025

Conversation

colltoaction
Copy link
Contributor

See #625.

@nwlandry
Copy link
Collaborator

@colltoaction --- thanks so much for the contribution! My biggest question is whether the default should be to read/write from a DiGraph. I think that what you've implemented makes sense as well --- maybe we could handle this with a kwarg or something?

@colltoaction
Copy link
Contributor Author

My understanding is that we always need a MultiGraph, except maybe for half-edges which I don't know.

we mean “pseudographs” in the greatest level of generality, allowing for loops, multiple edges between vertices, and dangling half-edges
https://ncatlab.org/nlab/show/hypergraph#hypergraphs_as_2colored_graphs

I guess using other classes of graphs we get something different than a hypergraph, is this a known structure?

@tlarock
Copy link
Collaborator

tlarock commented Nov 26, 2024

Thanks for the contribution @colltoaction ! Really useful functionality, I had actually been wondering about how to do this recently.

Wondering if it is possible to achieve this using the original create_using semantics, rather than conditioning on the user correctly setting G.graph.get("network-type") in the bipartite graph? I think we could check if create_using is of type xgi.DiHypergraph (others may reasonably disagree with this approach!), then maybe validate that direction is in the edge-data of the bipartite graph and raise an error if not.

My reasoning is that as written, we ask the user to specify they want a directed hypergraph in 3 different ways: (1) they have to set the "network-type" variable in the bipartite graph to the string "directed"; (2) they need to include "direction" in the edge data; and finally (3) they pass create_using=xgi.DiHypergraph. My suggestion would remove (1) and validate that they have done (2).

On the latter, my potentially wrong understanding is that if a user has not done (2) correctly (e.g., if they accidentaly typed "drection" as the edge-data in the bipartite graph), they will get a DiHypergraph with all heads without warning - is that correct?

Related, but separate: Do we want to allow users to construct simplicial complexes from bipartite graphs? Is that implemented separately? Related because if someone passed create_using=xgi.SimplicialComplex I think it would run fine, but it is not clear whether we intend that or not (to me, the documentation all reads as hypergraphs, not simplicial complexes). @nwlandry let me know if you think I should open a new issue for this one or if it is dealt with somehow.

@colltoaction
Copy link
Contributor Author

@tlarock for context, some of those decisions are part of the HIF format seen here:

My intention is to provide a HIF loader for NX that higher-order order libraries can reuse. I.e it should be the same to load from HIF JSON directly to XGI or through an intermediate NX graph then converting it.

@nwlandry
Copy link
Collaborator

@colltoaction @tlarock --- I thought more about the API for this and here is what I would propose:

  • have a create_using keyword with default None
  • If create_using is None, then Graph and MultiGraph get mapped to Hypergraph and DiGraph and MultiDiGraph get mapped to DiHypergraph
  • This default behavior can be overridden with create_using, but maybe there are limitations like, for instance, DiGraph variants can't be mapped to Hypergraph?

Copy link
Contributor Author

@colltoaction colltoaction left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the PR and would like to point out some comments below

tests/conftest.py Outdated Show resolved Hide resolved
xgi/convert/bipartite_graph.py Outdated Show resolved Hide resolved
xgi/convert/bipartite_graph.py Outdated Show resolved Hide resolved
@nwlandry
Copy link
Collaborator

@colltoaction --- I made some changes to simplify the API and to add the ability to convert a DiHypergraph to a bipartite DiGraph. Let me know what you think!

@colltoaction
Copy link
Contributor Author

@nwlandry your changes worked perfectly in my library after small tweaks. I switched from a MultiDiGraph to a DiGraph, so that makes me think what information could be missing.

Anyways it would be very helpful to have this!

@nwlandry
Copy link
Collaborator

@nwlandry your changes worked perfectly in my library after small tweaks. I switched from a MultiDiGraph to a DiGraph, so that makes me think what information could be missing.

Anyways it would be very helpful to have this!

Glad to hear! XGI handles multiedges with unique edge IDs, so we don't need multiedges in the bipartite directed graph. But I suppose that could be good to handle in the distant future!

@nwlandry nwlandry merged commit 55ecf68 into xgi-org:main Jan 18, 2025
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants