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

Restructure folder for generators and linalg #321

Merged
merged 13 commits into from
Apr 5, 2023

Conversation

maximelucas
Copy link
Collaborator

Following #319

@codecov
Copy link

codecov bot commented Apr 4, 2023

Codecov Report

Patch coverage: 97.34% and project coverage change: +0.05 🎉

Comparison is base (7ee8aa2) 90.29% compared to head (d848f70) 90.35%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #321      +/-   ##
==========================================
+ Coverage   90.29%   90.35%   +0.05%     
==========================================
  Files          36       41       +5     
  Lines        2926     2954      +28     
==========================================
+ Hits         2642     2669      +27     
- Misses        284      285       +1     
Impacted Files Coverage Δ
xgi/drawing/draw.py 72.39% <ø> (ø)
xgi/generators/classic.py 94.73% <ø> (-1.53%) ⬇️
xgi/generators/uniform.py 93.91% <0.00%> (ø)
xgi/readwrite/xgi_data.py 94.00% <ø> (ø)
xgi/dynamics/synchronization.py 81.96% <60.00%> (+0.30%) ⬆️
xgi/generators/random.py 83.07% <96.00%> (ø)
xgi/generators/simplicial_complexes.py 96.47% <96.47%> (ø)
xgi/linalg/laplacian_matrix.py 97.82% <97.82%> (ø)
xgi/linalg/hodge_matrix.py 97.91% <97.91%> (ø)
xgi/generators/__init__.py 100.00% <100.00%> (ø)
... and 4 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@maximelucas maximelucas marked this pull request as ready for review April 4, 2023 11:08
@maximelucas
Copy link
Collaborator Author

maximelucas commented Apr 4, 2023

It's already big enough to review I'd say.

Done:

  • divided linalg into: hypergraph_matrix, hodge_matrix, and laplacian_matrix
  • divided the generators into: classic, simple, lattice, random, uniform, simplicial_complexes
  • merge the uniform and nonuniform random_hypergraph() (this resulted in not have the option to pass a desired degree like the uniform version had, but we could add an external function to convert p to degree)
  • divided the tests accordingly

To do:

  • rename generators consistently

@nwlandry
Copy link
Collaborator

nwlandry commented Apr 4, 2023

Overall, looks good! Some high-level things:

  • Can you update the docs to match the change in structure? In addition, can you move utils and linalg into their own folders and update with the new module names? For reference, see PR Add clustering coefficient definitions #316.
  • The one nice thing about the uniform Erdös-Rényi hypergraph is that it had O(|V| +|E|) complexity which I needed for some larger hypergraphs. Is there a way to incorporate this in the random_hypergraph() function?

codecov.yml Outdated Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't know if I like the "hypergraph_matrix.py" name. Maybe break this into "incidence" and "adjacency"? But of course the intersection profile and degree matrices don't clearly belong to either. My concern is that these matrices aren't exclusively for hypergraphs in the way that we set them up.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not exclusively for hypergraphs in the sense that they also work for simplicial complexes? But SCs are HGs O:)
We could just call it "matrix.py", or "structure". It's not so clear how to break into less than 5 files indeed.

Copy link
Collaborator

@nwlandry nwlandry left a comment

Choose a reason for hiding this comment

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

Thanks for tackling this! A lot of good stuff here. I would say that the big thing is to add docs for the restructured files and there's just a few comments. Nice work!

@nwlandry
Copy link
Collaborator

nwlandry commented Apr 5, 2023

Hi @maximelucas - I had the opportunity to look over this PR and looks great! I hope you don't mind that I did the following:

  • I added the specific files relating to the modules that you added.
  • I added the uniform_erdos_renyi_hypergraph back in, and I will add this as an issue so that eventually we can port it over to the random module as fast_random_hypergraph or something.

@maximelucas
Copy link
Collaborator Author

Perfect, thanks Nich!

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