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

Support non-simplicial complexes #26

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

trevorkarn
Copy link

No description provided.

@codecov
Copy link

codecov bot commented Mar 29, 2021

Codecov Report

Merging #26 (e9e9b94) into master (eb20753) will decrease coverage by 2.72%.
The diff coverage is 29.41%.

❗ Current head e9e9b94 differs from pull request most recent head 5ddaa30. Consider uploading reports for the commit 5ddaa30 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master      #26      +/-   ##
==========================================
- Coverage   94.47%   91.74%   -2.73%     
==========================================
  Files          12       12              
  Lines         398      412      +14     
==========================================
+ Hits          376      378       +2     
- Misses         22       34      +12     
Impacted Files Coverage Δ
cechmate/filtrations/custom.py 0.00% <0.00%> (ø)
cechmate/solver.py 86.36% <23.07%> (-11.01%) ⬇️
cechmate/filtrations/base.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eb20753...5ddaa30. Read the comment docs.

@@ -59,6 +68,36 @@ def phat_diagrams(simplices, show_inf=False, verbose=True):

return dgms

def _assert_simplicial(simplices):
Copy link
Member

Choose a reason for hiding this comment

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

Please run black code formatter over these changes.


def diagrams(self, simplices=None, show_inf=False, verbose=True, simplicial=True):
"""
Redefine the Custom version of diagrams so that simplicial defaults to True, rather
Copy link
Member

Choose a reason for hiding this comment

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

Could you help me understand why this one should default to true while the others default to false?

Copy link
Author

Choose a reason for hiding this comment

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

My thought was that in all of the others, the simplicial complex is being generated in a way that we know it will be a simplicial complex. I thought it would be faster to pass False because we don't need to check it. My thought was not that the other ones are nonsimplicial, it's just that we don't need to assert that they are. For this one, we are generating the complex in a way that may not be simplicial. I assumed that most users would want the default behavior to be a simplicial complex, and they could choose to make something more subtle by passing simplicial=False

@sauln sauln changed the title Nonsimplicial Support non-simplicial complexes Mar 30, 2021
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