-
Notifications
You must be signed in to change notification settings - Fork 44
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
Assign operator #314
Assign operator #314
Conversation
b72bb92
to
65e2257
Compare
Coverage reportMain: 91.10% | PR: 91.12% | Diff: 0.02 ✅ |
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.
Great work :) Left some nits
temporian/core/event_set_ops.py
Outdated
def assign( | ||
self: EventSetOrNode, **others: EventSetOrNode | ||
) -> EventSetOrNode: | ||
"""Assign new features to an EventSet |
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.
missing fullstop, missing crosslink to EventSet ([EventSet
][temporian.EventSet])
If the name provided already exists on the EventSet, the feature is | ||
overriden. |
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.
If the feature names provided already exist on the EventSet, those features are replaced by the new ones?
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 replicated that functionality from pandas.
Inside the assign, I first drop the existing ones otherwise the glue would fail
>>> ab = a.assign(new_name=b) | ||
>>> ab | ||
indexes: [] | ||
features: [('A', int64), ('new_name', int64)] |
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.
normalize using ' or " for strings
temporian/core/operators/glue.py
Outdated
for name, other in others.items(): | ||
if len(other.schema.features) != 1: | ||
raise ValueError( | ||
"Cannot assign an EventSets with multiple features" |
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.
typo, an EventSet. but would change to: The assigned EventSets must have a single feature (since it also covers the case of it having 0)
... features={"B": [3, 4]}, | ||
... same_sampling_as=a, | ||
... ) | ||
>>> ab = a.assign(new_name=b) |
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.
adding an example with more than one feature would be useful
65e2257
to
24e3de1
Compare
24e3de1
to
a2b7ca0
Compare
Coverage reportMain: 91.10% | PR: 91.12% | Diff: 0.02 ✅ |
No description provided.