-
Notifications
You must be signed in to change notification settings - Fork 6
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 set-like methods to Session #984
base: master
Are you sure you want to change the base?
Conversation
2a82022
to
ddfd1d2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a general remark: update the tutorial ?
@@ -18,6 +18,9 @@ Backward incompatible changes | |||
New features | |||
^^^^^^^^^^^^ | |||
|
|||
* implemented :py:obj:`Session.union()`, :py:obj:`Session.intersection()` and :py:obj:`Session.difference()` to combine | |||
several sessions into one using set operations (closes :issue:`22`). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update the tutorial ?
During the last few months, I had the opportunity to discuss with some of our (unexperimented) users. They truly consider the tutorial as a Bible (not sure it's a good thing...).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I should and I will. I don't like doing that but if it's not in the tutorial, it might as well not exist for some (most?) of our users.
@@ -1388,6 +1388,93 @@ def apply(self, func, *args, **kwargs) -> 'Session': | |||
kind = kwargs.pop('kind', Array) | |||
return Session([(k, func(v, *args, **kwargs) if isinstance(v, kind) else v) for k, v in self.items()]) | |||
|
|||
def union(self, other): | |||
"""Returns session with the (set) union of this session objects with other objects. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a warning telling that using this method on two CheckedSession
objects will return a normal Session
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
return self[[k for k in self.keys() if k in other_keys]] | ||
|
||
def difference(self, other): | ||
"""Returns session with the (set) difference of this session objects and other objects. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same remark as for union()
return res | ||
|
||
def intersection(self, other): | ||
"""Returns session with the (set) intersection of this session objects with other objects. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same remark as for union()
>>> s1.intersection(['arr2', 'arr1']) | ||
Session(arr1, arr2) | ||
""" | ||
other_keys = set(other.keys() if isinstance(other, Session) else other) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you tested with CheckedSession
objects ?
CheckedSession
inherits from Session
so I see no reason it doesn't work but we never know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I didn't think of that. This is a VERY old branch I cleaned up and documented in the train. Didn't think of CheckedSession since they didn't exist at the time 😉.
No description provided.