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

Fixing things in drawing functions #476

Merged
merged 17 commits into from
Oct 25, 2023
Merged

Fixing things in drawing functions #476

merged 17 commits into from
Oct 25, 2023

Conversation

maximelucas
Copy link
Collaborator

@maximelucas maximelucas commented Oct 17, 2023

@codecov
Copy link

codecov bot commented Oct 17, 2023

Codecov Report

Attention: 21 lines in your changes are missing coverage. Please review.

Comparison is base (536f592) 92.03% compared to head (ac95a64) 92.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #476      +/-   ##
==========================================
- Coverage   92.03%   92.00%   -0.03%     
==========================================
  Files          60       60              
  Lines        4392     4429      +37     
==========================================
+ Hits         4042     4075      +33     
- Misses        350      354       +4     
Files Coverage Δ
xgi/drawing/layout.py 99.18% <100.00%> (-0.02%) ⬇️
xgi/drawing/draw_utils.py 88.42% <50.00%> (-0.19%) ⬇️
xgi/stats/__init__.py 85.46% <50.00%> (-0.42%) ⬇️
xgi/drawing/draw.py 77.94% <81.18%> (+1.62%) ⬆️

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

@thomasrobiglio
Copy link
Collaborator

thanks max! I'll check about #433

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@maximelucas
Copy link
Collaborator Author

maximelucas commented Oct 25, 2023

This is ready for review.

With the last commits, I rewrote draw_dihypergraph. The new implementation:

  • uses draw_nodes() to draw both types of nodes

  • uses individual FancyArrowPatch for the edges. This mimicks Networkx for directed graphs. I commented in the code why this seems to be the best solution (see this discussion). Arrows can be parametrised with arrowstyle, arrowsize, connectionstyle.

  • solves bugs when setting colors/linewidths/etc.

  • is now consistent with the other new functions, and drops dependency of _color_arg_to_dict and _scalar_arg_to_dict.

  • has more tests

  • has a new argument iterations to control the layout (related to Random edge position for dyads? #396). The previous layout was a spring layout computed on the "augmented projection", so that "edge"-nodes were sometimes on the outside, not at the barycenters. Now, the layout is computed as follows:

    1. compute a spring layout based solely on the original nodes.
    2. add the "edge"-nodes at the barycenters (at this point, nodes may overlap in larger networks)
    3. starting from this initial layout, run a spring layout on the whole thing, using iterations iterations. So 0 iterations will yield the layout computed in ii, while many iterations will yield the layout that we used to have.

    There is also a new pos argument so that user can play around. We could provide a special layout function for this in the future if we find a good way to do it.

  • has a new In Depth tutorial

Copy link
Collaborator

@thomasrobiglio thomasrobiglio left a comment

Choose a reason for hiding this comment

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

Thanks @maximelucas this looks super good! I have added minor comments. One more small thing:

I have noticed that we were inconsistent in the naming of the in-depth tutos (see pic). Can you:
In depth 2 - Drawing hyperedges -> In depth: drawing hyperedges
In Depth - Drawing DiHypergraphs -> In depth: drawing DiHypergraphs
(or something else to make the three names consistent?)
Screenshot 2023-10-25 alle 11 43 26

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

Good points Thomas, thanks! All addressed now I think.

@nwlandry
Copy link
Collaborator

Great work, @maximelucas!! Much needed changes and thanks for pushing this through!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants