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

Drawing hypergraphs with hyperdedges as convex hulls #320

Merged
merged 30 commits into from
Apr 5, 2023

Conversation

thomasrobiglio
Copy link
Collaborator

I have defined a new function called draw_hypergraph_hull that allows for drawing hypergraphs with hyperdedges of order > 1 as convex hulls. This is done using the ConvexHull function from scipy.spatial. The drawing style is inspired from that of HyperNetX but implemented differently and coherently with the rest of xgi. I have also pushed a tutorial showing the different setting that can be passed to the function.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@codecov
Copy link

codecov bot commented Apr 3, 2023

Codecov Report

Patch coverage: 92.30% and project coverage change: +3.76 🎉

Comparison is base (7d5accb) 86.39% compared to head (64e92d5) 90.16%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #320      +/-   ##
==========================================
+ Coverage   86.39%   90.16%   +3.76%     
==========================================
  Files          35       35              
  Lines        2757     2867     +110     
==========================================
+ Hits         2382     2585     +203     
+ Misses        375      282      -93     
Impacted Files Coverage Δ
xgi/drawing/draw.py 72.39% <92.30%> (+48.37%) ⬆️

... and 3 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

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

nwlandry commented Apr 3, 2023

One quick ask: can you delete the **2 from node_size**2 in draw_nodes? I think it would partially fix #272.

@maximelucas
Copy link
Collaborator

We could yea but I think the **2 is how it works in most of matplotlib, like the scatter function?
In any case, if we do change that, we'd need to adapt the default size value.

@leotrs
Copy link
Collaborator

leotrs commented Apr 4, 2023

It seems like accepting negative values is separate from whether or not we want to square them for scale. I'd leave that for a different PR.

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

leotrs commented Apr 5, 2023

Thank you so much @thomasrobiglio ! I've approved but I'll let @maximelucas decide when to merge in case there's anything else to do here.

@maximelucas
Copy link
Collaborator

Thanks Leo!

For completeness, the last thing would be to add a simple test for draw_hypergraph_hull, mimicking the other drawing tests here:
https://github.com/ComplexGroupInteractions/xgi/blob/main/tests/drawing/test_draw.py

Are you up for it @thomasrobiglio ?

Note that I could not implement the test on the number of elements and on the zorder for the dyads because the drawing of the hulls generates 2D lines as well.
@thomasrobiglio
Copy link
Collaborator Author

@maximelucas I have added the test. I could not implement also the test for the number of elements and zorder of the dyads because the drawing of the hulls also generated 2D lines (and the number of those is very initialisation dependent). For the moment I couldn't think of a solution to this.

@maximelucas
Copy link
Collaborator

Thanks Thomas, good to go for me!

@maximelucas maximelucas merged commit 759114e into xgi-org:main Apr 5, 2023
@nwlandry
Copy link
Collaborator

nwlandry commented Apr 5, 2023

Thanks so much @thomasrobiglio!! Looks awesome!!

@leotrs
Copy link
Collaborator

leotrs commented Apr 5, 2023

Congratulations @thomasrobiglio on your first merged PR. Hopefully first of many ;)

@iaciac
Copy link
Collaborator

iaciac commented Apr 5, 2023

Amazing, @thomasrobiglio! This is both what XGI deserves and what it needs right now. Thanks a lot

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.

None yet

5 participants