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

Added set-like methods to Session #984

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gdementen
Copy link
Contributor

No description provided.

Copy link
Collaborator

@alixdamman alixdamman left a 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`).
Copy link
Collaborator

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...).

Copy link
Contributor Author

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.
Copy link
Collaborator

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.

Copy link
Contributor Author

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.
Copy link
Collaborator

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.
Copy link
Collaborator

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)
Copy link
Collaborator

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.

Copy link
Contributor Author

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 😉.

@gdementen gdementen added this to the 0.34 milestone Sep 16, 2022
@gdementen gdementen self-assigned this Sep 20, 2022
@gdementen gdementen modified the milestones: 0.34, 0.35 Feb 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants