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

Update Wrapper #1712

Merged
merged 6 commits into from
Jan 17, 2024
Merged

Update Wrapper #1712

merged 6 commits into from
Jan 17, 2024

Conversation

varunagrawal
Copy link
Collaborator

Update the wrapper with new functionality supporting a new double-underscore (dunder) syntax. This makes wrapping classes that should exhibit some behavior in Python easier.

E.g. there is an ask in #1606 for KeySet to allow iteration and other container based operations. By adding __iter__ and __contains__ dunder methods to the gtsam.i file, we now support those operations.
This means we can now do this:

keys = graph1.keys()
for key in keys:
    if key in graph2.keys():
        print("key is in both graphs")

Consequently, fixes #1606.

cdcf23207 Merge pull request #163 from borglab/dunder-methods
22d429cc2 use STL functions instead of container methods
abe8818ab update pyparsing to 3.1.1
a9e896d0c add unit tests
535fe4f14 wrap supported dunder methods
079687eac parse dunder methods in interface file

git-subtree-dir: wrap
git-subtree-split: cdcf23207fbb03457c5d9dbfc2b0b57e515b5f3d
@varunagrawal varunagrawal added python Related to python wrapper wrapper Related to the wrapper labels Jan 16, 2024
@varunagrawal varunagrawal self-assigned this Jan 16, 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.

LGTM! Though I do think in gtwrap the __contains__ should use .find() instead of std::find but 👍

gtsam/gtsam.i Outdated
@@ -39,6 +39,11 @@ class KeyList {
void remove(size_t key);

void serialize() const;

// Specual dunder methods for Python wrapping
Copy link
Member

Choose a reason for hiding this comment

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

Typo: "Specual" -> "Special"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Corrected! I agree with the comment about .find() but until we move to C++20, many C++ STL containers won't support a find method. :(

@varunagrawal varunagrawal merged commit 2dfd15e into develop Jan 17, 2024
31 checks passed
@varunagrawal varunagrawal deleted the update-wrap branch January 17, 2024 04:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
python Related to python wrapper wrapper Related to the wrapper
Projects
None yet
Development

Successfully merging this pull request may close these issues.

KeySet class in python is missing methods
2 participants