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

fix #580 + related test #581

Merged
merged 5 commits into from
Sep 9, 2024
Merged

Conversation

thomasrobiglio
Copy link
Collaborator

@thomasrobiglio thomasrobiglio commented Sep 6, 2024

fixes #580

@thomasrobiglio thomasrobiglio changed the title fix #580 + related tests fix #580 + related test Sep 6, 2024
@maximelucas
Copy link
Collaborator

Thanks Thomas!

I think I would move the logic of this to remove_simplex_ids_from() instead. Because in general if a user wants to use remove_simplex_id(), this removed_ids() option is a bit surprising. And it's really a problem that only internal to remove_simplex_ids_from() because we remove several at one and it needs to be self-consistent. So maybe just similarly keep track of removed ids there, and then decide when to call remove_simplex_id based on that.

What do you think?

@thomasrobiglio
Copy link
Collaborator Author

Yes, makes more sense the way you say, on it.

supfaces_ids = self._supfaces_id(self._edge[id])
for sup_id in supfaces_ids:
self._remove_simplex_id(sup_id)
self.remove_simplex_id(sup_id)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't this was wrong before so I wouldn't touch it, unless you think it's more efficient?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good catch, thx

thomasrobiglio and others added 3 commits September 9, 2024 10:58
Co-authored-by: Maxime Lucas <maximelucas@users.noreply.github.com>
Co-authored-by: Maxime Lucas <maximelucas@users.noreply.github.com>
@thomasrobiglio thomasrobiglio merged commit 4d26fc8 into xgi-org:main Sep 9, 2024
22 checks passed
@thomasrobiglio thomasrobiglio deleted the fix-#580 branch September 9, 2024 09:13
thomasrobiglio added a commit to thomasrobiglio/xgi that referenced this pull request Sep 9, 2024
thomasrobiglio added a commit that referenced this pull request Sep 12, 2024
* feat: function + test

* style: formatted with black

* fix: iteration over orders

* fix: removed iteration across orders

* test: added test for k=0, related to #581

* style: black

* docs: added explanation + link to ref

* docs: rephrased definition

* moved function to `convert.simplex` + created file for test

* added k_skeleton to api rst file

* use `max_edge_order` function

* created cut_to_order + modified k_skeleton + tests

* added cut_to_order to api

* style: black

* minor change for consistency with other functions

* modified recipe

* changed formatting in the recipe
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.

remove_simplex_ids() raises error when removing all simplices from a SC
2 participants