xgi-org / xgi

CompleX Group Interactions (XGI) is a Python package for higher-order networks.
https://xgi.readthedocs.io
Other
171 stars 27 forks source link

Simplify dihypergraph #541

Closed nwlandry closed 1 week ago

nwlandry commented 1 month ago

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

codecov[bot] commented 1 month ago

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 1 commits behind head on main.

Files Patch % Lines
xgi/core/views.py 96.87% 2 Missing :warning:
Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

nwlandry commented 3 weeks ago

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

maximelucas commented 3 weeks ago

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 commented 2 weeks ago

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:

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 commented 1 week ago

Thanks!