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

Add ability to generate ER hypergraphs without multiedges #596

Merged
merged 18 commits into from
Oct 10, 2024

Conversation

nwlandry
Copy link
Collaborator

@nwlandry nwlandry commented Sep 16, 2024

This PR fixes #339.

Copy link

codecov bot commented Sep 16, 2024

Codecov Report

Attention: Patch coverage is 84.21053% with 6 lines in your changes missing coverage. Please review.

Project coverage is 93.17%. Comparing base (5dc6f4f) to head (d2994fb).
Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
xgi/generators/uniform.py 82.85% 6 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##             main     #596    +/-   ##
========================================
  Coverage   93.16%   93.17%            
========================================
  Files          60       62     +2     
  Lines        4523     4701   +178     
========================================
+ Hits         4214     4380   +166     
- Misses        309      321    +12     

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

@nwlandry
Copy link
Collaborator Author

@arashbm --- does this fix the issue you raised?

@nwlandry nwlandry requested review from maximelucas and removed request for thomasrobiglio September 30, 2024 22:11
@nwlandry
Copy link
Collaborator Author

nwlandry commented Oct 7, 2024

@maximelucas --- are you able to review this?

@maximelucas
Copy link
Collaborator

Mainly a few comments about clarifying the code with comments!

nwlandry and others added 4 commits October 9, 2024 09:53
Co-authored-by: Maxime Lucas <maximelucas@users.noreply.github.com>
@nwlandry
Copy link
Collaborator Author

nwlandry commented Oct 9, 2024

@maximelucas --- I believe I addressed all of your comments!


Notes
-----
Because XGI only stores edges as sets, if self-loops are allowed,
Copy link
Collaborator

Choose a reason for hiding this comment

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

But there's no option to allow (or not) self-loops is this function, right? I'm a bit confused. In any case, since that singleton {0} is always removed by the method, does it matter?

The multiedges option is not linked to that correct?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will try to clarify this sentence!

@maximelucas
Copy link
Collaborator

Thanks for the explanations! I'm still confused about some things, see above. Sorry for being a pain 😅 As long as the advantages are clear (and ideally why they are) we're good I guess. And that each method is clear from its own documentation.

If I understand a bit better, the index in "artificial/internal" in the sense that is just runs over all possible edges of size m among n nodes, with/without caring for the order (and hence allowing or not multiedges).

nwlandry and others added 6 commits October 10, 2024 12:16
Co-authored-by: Maxime Lucas <maximelucas@users.noreply.github.com>
Co-authored-by: Maxime Lucas <maximelucas@users.noreply.github.com>
Co-authored-by: Maxime Lucas <maximelucas@users.noreply.github.com>
Co-authored-by: Maxime Lucas <maximelucas@users.noreply.github.com>
Co-authored-by: Maxime Lucas <maximelucas@users.noreply.github.com>
Co-authored-by: Maxime Lucas <maximelucas@users.noreply.github.com>
@nwlandry
Copy link
Collaborator Author

nwlandry commented Oct 10, 2024

Thanks for the explanations! I'm still confused about some things, see above. Sorry for being a pain 😅 As long as the advantages are clear (and ideally why they are) we're good I guess. And that each method is clear from its own documentation.

If I understand a bit better, the index in "artificial/internal" in the sense that is just runs over all possible edges of size m among n nodes, with/without caring for the order (and hence allowing or not multiedges).

Yes, that's correct! I'll take a look at your more involved comments (hopefully today!) Thanks for the review!

@nwlandry nwlandry requested a review from maximelucas October 10, 2024 18:52
@nwlandry
Copy link
Collaborator Author

Thanks for all the comments!!

@nwlandry nwlandry merged commit f2e2241 into main Oct 10, 2024
24 checks passed
@nwlandry nwlandry deleted the fix-erdos-renyi branch October 10, 2024 20:35
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.

uniform_erdos_renyi_hypergraph produces way too many edges
2 participants