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 lost of tests for SimplicialComplex class #245

Merged
merged 10 commits into from
Dec 8, 2022
Merged

Added lost of tests for SimplicialComplex class #245

merged 10 commits into from
Dec 8, 2022

Conversation

maximelucas
Copy link
Collaborator

@maximelucas maximelucas commented Dec 7, 2022

  • added lots of tests
  • changed the behaviour of inherited Hypergraph methods such as add_edge: instead of raising an error and not adding the simplex, we now add the simplex and raise a warning.
  • a consequence of the previous point is that some HG methods that did not work for SCs now do (and raise a warning): e.g. copy()
  • added tests to check the new warnings and other old warnings
  • fixed an off-by-one in update_uid_counter bug in add_simplices_from when specifying uid #243

The cleanup of the SimplicialComplex is not finished yet but I feel like this PR is getting big already.
There is one test that does not pass, due to #244. I feel like fixing it would be a lot of code change and would be better in another PR. (The test is new and the fact that it does not pass just highlights a pre-existing bug)

@leotrs
Copy link
Collaborator

leotrs commented Dec 8, 2022

Thanks for all your work here. So your suggestion is that we merge this now as is and then fix the broken test in a different PR?

@maximelucas
Copy link
Collaborator Author

maximelucas commented Dec 8, 2022

Yes exactly. Because fixing it will require to rewrite the add_simplices_from function as non-recursive and change the order in which simplices are added, so we will probably need to update existing tests too. Is that allowed by Github?

@leotrs
Copy link
Collaborator

leotrs commented Dec 8, 2022

Great! I'd suggest we merge this and then get on working the next PR asap.

@maximelucas maximelucas merged commit 7457018 into main Dec 8, 2022
@maximelucas maximelucas deleted the sc-tests branch December 8, 2022 13:38
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