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

Rewrite draw_hyperedges() #456

Merged
merged 30 commits into from
Oct 16, 2023
Merged

Rewrite draw_hyperedges() #456

merged 30 commits into from
Oct 16, 2023

Conversation

maximelucas
Copy link
Collaborator

@maximelucas maximelucas commented Aug 24, 2023

Example customisation:

ax, (dyad_collection, edge_collection) = xgi.draw_hyperedges(
    H,
    pos=pos,
    dyad_color=[3, 1, 4],
    edge_fc=H.edges.size,
    dyad_color_cmap="viridis",
    edge_fc_cmap="tab10"
)

plt.colorbar(dyad_collection, label="dyads")
plt.colorbar(edge_collection, label="edges")
Screenshot 2023-08-25 at 00 10 17

@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 Aug 25, 2023

Codecov Report

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

Comparison is base (1b8df54) 91.90% compared to head (4e51373) 92.03%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #456      +/-   ##
==========================================
+ Coverage   91.90%   92.03%   +0.12%     
==========================================
  Files          60       60              
  Lines        4375     4392      +17     
==========================================
+ Hits         4021     4042      +21     
+ Misses        354      350       -4     
Files Coverage Δ
xgi/drawing/draw.py 76.32% <98.11%> (+1.99%) ⬆️
xgi/drawing/draw_utils.py 88.61% <83.33%> (-1.91%) ⬇️

... and 1 file with indirect coverage changes

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

@maximelucas
Copy link
Collaborator Author

Two items required choices:

  1. the rewriting of the function made it had to "rescale" the cmap like we used to do to avoid extreme values to be mapped to very light colors. For example, the default cmap was Blues, and the lowest orders would be shown as very very light blue (hardly visible). I chose to have a default cmap that does not start with white ("crest" from Seaborn).
  2. the new use of PatchCollection prevents us to use different zorder for different edges (of different sizes) like we used to do. The trick is that it plots the patches in the order that they are given. I reordered the edges so that larger edges are plotted first (below).
  3. Related to 1, I'm unsure if we want to keep showing different orders with different colors by default. It looks nice for small hypergraphs, but for larger ones I feel it make things more confused.

I reran the tutorial notebooks so you can go check the new look there.

If we are happy with this implementation, I will update xgi.draw_simplices() to be consistent (this will fix the tutorial notebooks) and add more tests.

@nwlandry
Copy link
Collaborator

Great work, @maximelucas, and thanks for implementing such an awesome update to the drawing functionality! I've had a look and I think it is time to merge this. Before you do, though, can you correct the tutorial errors and change the draw_simplices function?

@maximelucas maximelucas marked this pull request as ready for review October 16, 2023 10:45
@maximelucas
Copy link
Collaborator Author

I think this is ready now. I updated draw() and draw_simplices() so that they are consistent with draw_hyperedges().

The functions _color_arg_to_dict() and _scalar_arg_to_dict() are not used in the new versions, but are still used in draw_dihypergraph(), draw_multilayer(), and draw_hypergraph_hull(). These functions might need to be updated for consistency. Also the way they handle the kwargs. I think this PR is already big enough so I would l create new issues and leaves this other PRs.

The only thing left: decide if we keep the current default cmap (crest_r) for edges or not.
I managed to create another cmap candidate which is a sequential blue going from not too light to not too dark:
Screenshot 2023-10-16 at 12 45 59

Here are 3 examples of different sizes with different densities and edges sizes with this cmap:

Screenshot 2023-10-16 at 12 46 16
Screenshot 2023-10-16 at 12 46 11
Screenshot 2023-10-16 at 12 46 06

Note how the original cmap is more saturated that what is shown there: that's because we use a default alpha=0.4 when plotting. In any case, I think this cmap might look better for the larger examples, but it looks a bit too light/transparent in the smaller example.

Let me know what you think on the cmap.

@nwlandry
Copy link
Collaborator

Looks great, @maximelucas!! Thanks for your hard work on this!

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.

easy colorbar plotting Speed up draw_xgi_nodes and draw_xgi_hyperedges
2 participants