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

Non recursive add simplex #247

Merged
merged 18 commits into from
Dec 9, 2022
Merged

Non recursive add simplex #247

merged 18 commits into from
Dec 9, 2022

Conversation

maximelucas
Copy link
Collaborator

@maximelucas maximelucas commented Dec 8, 2022

  • rewrite of add_simplices_from so that is is non-recursive.
  • updated tests

With the rewrite, we first add all input simplices, store all faces, and add the faces add the end. It used to add each input simplex with its faces, so this changes the order in which simplices are added. This new order solves #244

Notes:

  • to avoid too much redundancy and slowness, we add the set of faces. This seems to sometimes randomly change the order of simplices.
  • one test does not pass: test_boundary_matrix. I think it's cause by the change in order which causes simplices to have different IDs. Might need @Marconurisso to check on this one.
  • need to check if remove_simplex_ids_from does not enforce removing subfaces #85 is still a problem
  • I commented out some test lines that were catching warning when adding simplices because they were making the tests infinitely slow. I had to ctrl+c my way out everytime. No idea why.

@maximelucas maximelucas marked this pull request as ready for review December 9, 2022 13:32
@maximelucas
Copy link
Collaborator Author

@nwlandry
Copy link
Collaborator

nwlandry commented Dec 9, 2022

I did a few tests of my own, adding/removing simplices, etc. and everything works flawlessly! This is great work!!

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