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

Added cleanup method to all classes #440

Merged
merged 12 commits into from
Aug 4, 2023
Merged

Added cleanup method to all classes #440

merged 12 commits into from
Aug 4, 2023

Conversation

nwlandry
Copy link
Collaborator

@nwlandry nwlandry commented Aug 1, 2023

Fixes #195.

This PR

  • Adds the cleanup() method to SimplicialComplex and DiHypergraph and fixes several bugs in the cleanup() method for Hypergraph.
  • Adds unit tests.
  • Fixes the strong node removal in Issue Strong node removal breaks the internal Hypergraph data structure #445.
  • Adds the isolates() method to the DiNodeView class.
  • Adds the connected kwarg to cleanup() for hypergraphs and simplicial complexes, which removes nodes/edges not in the largest component.
  • is_frozen is now a property.

@codecov
Copy link

codecov bot commented Aug 1, 2023

Codecov Report

Patch coverage: 98.16% and project coverage change: +0.89% 🎉

Comparison is base (ea1aaf6) 90.94% compared to head (7435da9) 91.83%.
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #440      +/-   ##
==========================================
+ Coverage   90.94%   91.83%   +0.89%     
==========================================
  Files          59       60       +1     
  Lines        4076     4251     +175     
==========================================
+ Hits         3707     3904     +197     
+ Misses        369      347      -22     
Files Changed Coverage Δ
xgi/core/simplicialcomplex.py 85.66% <96.22%> (+2.39%) ⬆️
xgi/core/dihypergraph.py 98.12% <100.00%> (+0.11%) ⬆️
xgi/core/diviews.py 83.60% <100.00%> (+0.18%) ⬆️
xgi/core/hypergraph.py 93.54% <100.00%> (+6.29%) ⬆️
xgi/utils/utilities.py 95.28% <100.00%> (+0.23%) ⬆️

... and 14 files with indirect coverage changes

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

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@nwlandry nwlandry marked this pull request as ready for review August 3, 2023 19:47
@maximelucas
Copy link
Collaborator

maximelucas commented Aug 4, 2023

Thanks Nich, looks good. Good idea this connected argument!

It's a bit long so I didn't check every single line. Just a few comments.
Also same usual comment, not to fix now: it feels like we have a lot of (almost) redundant code between SC, HG, and DiHG.

@nwlandry
Copy link
Collaborator Author

nwlandry commented Aug 4, 2023

Thanks Nich, looks good. Good idea this connected argument!

It's a bit long so I didn't check every single line. Just a few comments. Also same usual comment, not to fix now: it feels like we have a lot of (almost) redundant code between SC, HG, and DiHG.

I agree - my goal is to get things working and then we can focus on inheritance/sharing functionality. One of the issues that I ran into was that because SimplicialComplex uses frozen sets, we can only do strong node removal. At that point, it made sense to just implement remove_ node and remove_nodes_from in SimplicialComplex.

@maximelucas
Copy link
Collaborator

Thanks for the explanations, looks good :)

@nwlandry
Copy link
Collaborator Author

nwlandry commented Aug 4, 2023

Thanks for the review, @maximelucas!! I appreciate it :)

@nwlandry nwlandry merged commit ab2a2c7 into main Aug 4, 2023
@nwlandry nwlandry deleted the cleanup-cleanup branch August 4, 2023 12:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants