Closed nwlandry closed 5 months ago
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 :warning: |
xgi/drawing/layout.py | 88.88% | 2 Missing :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
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?
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?
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 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 indraw_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.
Check out this pull request on
See visual diffs & provide feedback on Jupyter Notebooks.
Powered by ReviewNB
I have now implemented all of your suggestions, @maximelucas. Should be ready to re-review.
Thanks for the suggestions, @maximelucas! It looks like I need to take more time to address these concerns, and I'll let you know when I'm done!
@maximelucas, I believe I addressed all of your comments! Thanks for such a thorough and helpful review! I did the following:
draw_undirected_dyads
and draw_directed_dyads
standalone functions and made them available to the user.Thanks! Those are quick small comments for the last changes, I'll have a better look at the entire code soon.
@maximelucas, are you still able to review this? Thanks so much!
I'll do that next week!
Great! Thanks so much!
Some more minor comments.. we're almost there, it's a lot of code 😄 I'll check the rest of the code later today.
@maximelucas thanks so much! Let me know when you're done and I'll start addressing your comments!
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!
@maximelucas, thanks for the review! A quick overview of my changes based on your comments:
edge_positions_from_barycenters
as a more general replacement of the old functionality.draw_bipartite
on an undirected hypergraph in Tutorial 5edge_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.
Looks good, thanks so much for the work!
This PR does the following:
draw_dihypergraph
and moves its contents todraw_bipartite
.draw_bipartite
now handlesHypergraph
andDiHypergraph
objects.bipartite_spring_layout
position function.