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 simpliciality measures #587

Merged
merged 16 commits into from
Oct 9, 2024
Merged

Added simpliciality measures #587

merged 16 commits into from
Oct 9, 2024

Conversation

nwlandry
Copy link
Collaborator

@nwlandry nwlandry commented Sep 8, 2024

This PR adds all the functionality from this repo.

Copy link

codecov bot commented Sep 8, 2024

Codecov Report

Attention: Patch coverage is 95.56962% with 7 lines in your changes missing coverage. Please review.

Project coverage is 93.24%. Comparing base (5ff749e) to head (6ba8b6e).
Report is 14 commits behind head on main.

Files with missing lines Patch % Lines
xgi/algorithms/simpliciality.py 95.95% 4 Missing ⚠️
xgi/stats/nodestats.py 90.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #587      +/-   ##
==========================================
+ Coverage   93.13%   93.24%   +0.10%     
==========================================
  Files          60       62       +2     
  Lines        4503     4679     +176     
==========================================
+ Hits         4194     4363     +169     
- Misses        309      316       +7     

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

@nwlandry nwlandry requested a review from tlarock September 9, 2024 06:22
Copy link
Collaborator

@tlarock tlarock left a comment

Choose a reason for hiding this comment

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

Looks good overall and tests appear to pass. Worth considering how we structure the return of the functions, since using exception handling rather than conditionals is a design choice. One function appears to be duplicated. Otherwise good to go!

xgi/algorithms/simpliciality.py Outdated Show resolved Hide resolved
xgi/algorithms/simpliciality.py Outdated Show resolved Hide resolved
@nwlandry
Copy link
Collaborator Author

Thanks for the review, @tlarock! Someone pointed out a minor error in my simpliciality paper, so I'm determining what to do with that first before making your suggested changes.

@nwlandry
Copy link
Collaborator Author

nwlandry commented Oct 3, 2024

Hi @tlarock --- I believe I addressed all of your comments. The URL checker fails, but this was addressed in another PR so it will go away once we merge into the main branch.

@nwlandry nwlandry requested a review from tlarock October 3, 2024 17:47
Copy link
Collaborator

@tlarock tlarock left a comment

Choose a reason for hiding this comment

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

Very close! Sorry to be a pain about the documentation, but hopefully easier to clairfy now than later :) Of course feel free to take or leave

xgi/algorithms/simpliciality.py Outdated Show resolved Hide resolved
xgi/algorithms/simpliciality.py Show resolved Hide resolved
@nwlandry
Copy link
Collaborator Author

nwlandry commented Oct 7, 2024

@tlarock --- I believe I addressed your comments! I added more extensive documentation in the Notes section of the docstrings. Lmk what you think!

@tlarock
Copy link
Collaborator

tlarock commented Oct 9, 2024

Looks great @nwlandry! Ready for merging on my end.

@nwlandry nwlandry merged commit 69865e6 into main Oct 9, 2024
23 of 24 checks passed
@nwlandry nwlandry deleted the simpliciality branch October 9, 2024 12: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