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

Add the ability to draw hypergraphs as a bipartite graph #492

Merged
merged 45 commits into from
Apr 22, 2024

Conversation

nwlandry
Copy link
Collaborator

@nwlandry nwlandry commented Nov 3, 2023

This PR does the following:

  • Removes draw_dihypergraph and moves its contents to draw_bipartite.
  • The draw_bipartite now handles Hypergraph and DiHypergraph objects.
  • Added the bipartite_spring_layout position function.

@nwlandry nwlandry mentioned this pull request Nov 3, 2023
Copy link

codecov bot commented Nov 3, 2023

Codecov Report

Attention: Patch coverage is 91.12426% with 15 lines in your changes are missing coverage. Please review.

Project coverage is 92.29%. Comparing base (0f9be8d) to head (730bd74).

Files Patch % Lines
xgi/drawing/draw.py 91.33% 13 Missing ⚠️
xgi/drawing/layout.py 88.88% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #492      +/-   ##
==========================================
+ Coverage   92.16%   92.29%   +0.13%     
==========================================
  Files          60       60              
  Lines        4418     4493      +75     
==========================================
+ Hits         4072     4147      +75     
  Misses        346      346              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@maximelucas
Copy link
Collaborator

Just a thought: conceptually, could this function just be the same as the draw_dihypergraph with the edge markers, except that the links are drawn without arrows and the layout would be different?

@nwlandry
Copy link
Collaborator Author

nwlandry commented Nov 3, 2023

Mmmmm. This is a great point. I think you're right. Would it make sense to group them together, call it draw_bipartite and have different handling for directed and undirected hypergraphs?

@maximelucas
Copy link
Collaborator

I thought that could be a possibility. Worth thinking about. The current dihypergraph one only make sense as a bipartite if the edge markers are drawn though, I'd say.

Maybe directedness and "bipartite-ness" are two somewhat independent elements here.
Could the arrow drawing be included as a directed option in draw_hyperedges? Not sure that would include the edge markers.

And then the draw_bipartite could just be about the layout really. Either the current layout we have in the dihypergraph one, or a more bipartitey layout, the user could choose. And draw directed or undirected edges.

@nwlandry
Copy link
Collaborator Author

I thought that could be a possibility. Worth thinking about. The current dihypergraph one only make sense as a bipartite if the edge markers are drawn though, I'd say.

Maybe directedness and "bipartite-ness" are two somewhat independent elements here. Could the arrow drawing be included as a directed option in draw_hyperedges? Not sure that would include the edge markers.

And then the draw_bipartite could just be about the layout really. Either the current layout we have in the dihypergraph one, or a more bipartitey layout, the user could choose. And draw directed or undirected edges.

I have refactored the function so that it accepts both directed and undirected hypergraphs. Because of this, draw_dihypergraph is now unnecessary and so I removed it.

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@nwlandry nwlandry requested a review from maximelucas February 26, 2024 17:30
@nwlandry
Copy link
Collaborator Author

I have now implemented all of your suggestions, @maximelucas. Should be ready to re-review.

@maximelucas
Copy link
Collaborator

I'll do that next week!

@nwlandry
Copy link
Collaborator Author

nwlandry commented Apr 9, 2024

Great! Thanks so much!

xgi/drawing/draw.py Outdated Show resolved Hide resolved
xgi/drawing/draw.py Outdated Show resolved Hide resolved
xgi/drawing/draw.py Outdated Show resolved Hide resolved
@maximelucas
Copy link
Collaborator

Some more minor comments.. we're almost there, it's a lot of code 😄 I'll check the rest of the code later today.

@nwlandry
Copy link
Collaborator Author

@maximelucas thanks so much! Let me know when you're done and I'll start addressing your comments!

xgi/drawing/draw.py Outdated Show resolved Hide resolved
@maximelucas
Copy link
Collaborator

I think I checked all of it 😛
I would just add an example somewhere of an undirected one, and raise an issue to create a new function with the "classic" graph bipartite layout, and maybe also with the barycenter layout.
Thanks for all the work!

nwlandry and others added 8 commits April 17, 2024 16:51
Co-authored-by: Maxime Lucas <maximelucas@users.noreply.github.com>
Co-authored-by: Maxime Lucas <maximelucas@users.noreply.github.com>
Co-authored-by: Maxime Lucas <maximelucas@users.noreply.github.com>
@nwlandry
Copy link
Collaborator Author

@maximelucas, thanks for the review! A quick overview of my changes based on your comments:

  • changed all of the dyads to be colored by the edge size (with "crest_r") by default
  • Added error handling in a few places
  • Added a new function edge_positions_from_barycenters as a more general replacement of the old functionality.
  • Added an example of using draw_bipartite on an undirected hypergraph in Tutorial 5
  • Added a few examples of the new edge_positions_from_barycenters in Tutorial 5 and In Depth 3.

At this point, I believe that I've addressed all your comments. Let me know if there are other things you would like me to change.

@maximelucas
Copy link
Collaborator

Looks good, thanks so much for the work!

@nwlandry nwlandry merged commit 46f3be3 into main Apr 22, 2024
24 checks passed
@nwlandry nwlandry deleted the add-draw-bipartite branch April 22, 2024 13:09
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.

2 participants