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

Simplify dihypergraph #541

Merged
merged 20 commits into from
Jun 24, 2024
Merged

Simplify dihypergraph #541

merged 20 commits into from
Jun 24, 2024

Conversation

nwlandry
Copy link
Collaborator

@nwlandry nwlandry commented May 26, 2024

This PR changes the internals of DiHypergraph so that it is more consistent with the other classes. This is an attempt to start addressing #379. This PR

  • Replaces _node_in and _node_out with _node where _node[n] is of the form {"in": {tail}, "out": {head}}
  • Replaces _edge_in and _edge_out with _edge where _edge[n] is of the form {"in": {}, "out": {}}
  • Removes DiIDView since now we can simply inherit from IDView.
  • Moves DiNodeView and DiEdgeView to views.py and deletes diviews.py
  • Moves tests for directed views to test_views.py
  • Removes unnecessary imports in the unit tests.

Copy link

codecov bot commented May 26, 2024

Codecov Report

Attention: Patch coverage is 98.68421% with 2 lines in your changes missing coverage. Please review.

Project coverage is 92.49%. Comparing base (c0f7593) to head (c4b8a73).
Report is 44 commits behind head on main.

Files with missing lines Patch % Lines
xgi/core/views.py 96.87% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #541      +/-   ##
==========================================
+ Coverage   92.35%   92.49%   +0.14%     
==========================================
  Files          60       59       -1     
  Lines        4499     4346     -153     
==========================================
- Hits         4155     4020     -135     
+ Misses        344      326      -18     

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

@nwlandry nwlandry requested a review from maximelucas May 27, 2024 11:29
@nwlandry
Copy link
Collaborator Author

@maximelucas - when do you think you will be able to take a look at this? Thanks!

xgi/core/views.py Outdated Show resolved Hide resolved
xgi/core/views.py Outdated Show resolved Hide resolved
@maximelucas
Copy link
Collaborator

I've had a look now, thanks!
I left comments which are mostly questions for my own understanding.

The main one may be about about the choice between having H._node[node_id]["in"] and not H._nodes["in"][node_id]. I'm just wondering which one will be easier to generalise (don't know what I would choose). The first (current) option allows the call to H._[node_id] to be valid like before, but make it a bit more lengthy to access all "in" nodes. By the way, I'm just realising, what is an in-node again? 😛

If the API for the user is unchanged it's not needed, but otherwise we should document this somewhere.

@nwlandry
Copy link
Collaborator Author

I've had a look now, thanks! I left comments which are mostly questions for my own understanding.

The main one may be about about the choice between having H._node[node_id]["in"] and not H._nodes["in"][node_id]. I'm just wondering which one will be easier to generalise (don't know what I would choose). The first (current) option allows the call to H._[node_id] to be valid like before, but make it a bit more lengthy to access all "in" nodes. By the way, I'm just realising, what is an in-node again? 😛

If the API for the user is unchanged it's not needed, but otherwise we should document this somewhere.

Thanks for the review! I've thought quite a bit about your suggestion and I'd like to leave the structure (H._edge[e]["in] for example) as is for the following reasons:

  • It seems natural for an edge ID to come with a list of all of its in- and out-connections
  • The goal of this PR is to move us toward full inheritance and I think if we wanted to overload some algorithm definitions, it would be more natural for an edge ID to return a full edge, whether undirected or directed.

As far as your question goes, IMO it's most natural to think about it in the bipartite sense. If a node is specified as an input (in the tail) in a directed edge, that edge will add to its out-degree.

@maximelucas
Copy link
Collaborator

Thanks!

@nwlandry nwlandry merged commit 3d5aacc into main Jun 24, 2024
24 checks passed
@nwlandry nwlandry deleted the simplify-dihypergraph branch June 24, 2024 15:02
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