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

Parse dunder methods in interface file #163

Merged
merged 5 commits into from
Jan 15, 2024
Merged

Parse dunder methods in interface file #163

merged 5 commits into from
Jan 15, 2024

Conversation

varunagrawal
Copy link
Collaborator

Added support for specifying particular double-underscore aka dunder methods. This allows giving support for those methods particularly for classes that should behave like python containers e.g. gtsam::KeySet.

This will be used to fix borglab/gtsam#1606

@varunagrawal varunagrawal self-assigned this Jan 15, 2024
Copy link
Member

@gchenfc gchenfc left a comment

Choose a reason for hiding this comment

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

Much appreciated feature!
See comments, but as a whole much like!

gtwrap/pybind_wrapper.py Outdated Show resolved Hide resolved
tests/expected/python/class_pybind.cpp Outdated Show resolved Hide resolved
@varunagrawal varunagrawal requested a review from gchenfc January 15, 2024 18:18
Copy link
Member

@gchenfc gchenfc left a comment

Choose a reason for hiding this comment

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

See unresolved comment

@varunagrawal varunagrawal requested a review from gchenfc January 15, 2024 21:55
Copy link
Member

@gchenfc gchenfc left a comment

Choose a reason for hiding this comment

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

See comment

elif method.name == 'contains':
function_call = f"return self->find({method.args.args_list[0].name}) != self->end();"
function_call = f"return std::find(self->begin(), self->end(), {method.args.args_list[0].name}) != self->end();"
Copy link
Member

Choose a reason for hiding this comment

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

Sorry for the back-and-forth, but realizing that std::find(..) is always O(n) whereas for many data structures (e.g. std::set) self->find(obj) should be O(1), so probably using the ->find() version would be better in this case? Overkill approach I guess would be SFINAE, but that seems excessive in this case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Make up your mind, my guy 😂

I figured the find method would be faster but whoever designed the STL didn't think to make find a method for vectors and lists.

Also, this feature is not crucial. If people want efficiency, then cast to a Python set object (which is easily doable now since we support iteration).

@varunagrawal varunagrawal merged commit cdcf232 into master Jan 15, 2024
8 checks passed
@varunagrawal varunagrawal deleted the dunder-methods branch February 22, 2024 21: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.

KeySet class in python is missing methods
2 participants