Added before and after operators #316
Conversation
javiber
force-pushed
the
before-after-operator
branch
from
be89f88
to
95a5579
Compare
Coverage reportMain: 91.12% | PR: 91.15% | Diff: 0.03 ✅ |
temporian/core/event_set_ops.py
Outdated
| self: EventSetOrNode, | ||
| timestamp: Union[int, float, datetime], | ||
| ) -> EventSetOrNode: | ||
| """Filter events [`EventSet`][temporian.EventSet] that happened before a |
There was a problem hiding this comment.
Nit: Some of the functions use the 3rd person e.g. "Filters events ...".
| timestamps. | ||
|
|
||
| The comparison is strict, meaning that the obtained timestamps would be | ||
| less than (`<`) the provided timestamp. |
There was a problem hiding this comment.
Would it make sense to say something like?:
This operation is equivalent to "input.filter(input.timestamps() < timestamp)"
There was a problem hiding this comment.
I can add it, do you think I should also add a link to filter?
temporian/core/event_set_ops.py
Outdated
| self: EventSetOrNode, | ||
| timestamp: Union[int, float, datetime], | ||
| ) -> EventSetOrNode: | ||
| """Filter events [`EventSet`][temporian.EventSet] that happened before a |
There was a problem hiding this comment.
I am not a native speaker, but I think "filter events before A" might be understood as removing the ones before A (i.e. the opposite of this operator). I wonder if this could be made less ambiguous.
There was a problem hiding this comment.
I would understand "filter out..." as the opposite, since I'm not a native speaker either, run it through chatGPT the question:
Given the following documentation of a function called f:
"Filters events that happened before a particular timestamp."
What do you understand that the function should do?
And it understood the correct thing:
def f(events, timestamp):
# Filter events that happened before the specified timestamp
filtered_events = [event for event in events if event['timestamp'] < timestamp]
return filtered_events
| ``` | ||
|
|
||
| Args: | ||
| timestamp: EventSet with a single boolean feature. |
There was a problem hiding this comment.
What about something like: Strict timestamp upper boundary (or something like that).
|
|
||
| if isinstance(timestamp, datetime): | ||
| if not input.schema.is_unix_timestamp: | ||
| raise ValueError( |
There was a problem hiding this comment.
Do we want the opposite to be allowed? e.g. timestamp is a float and the input is a unix timestamp.
There was a problem hiding this comment.
That's a good question, I didn't want to be too restrictive but if you feel that it's a good restriction I'm happy to add it.
My only concern was if the user can somehow get the internal timestamps from an eventset they would be floats because that's how we represent them but I don't know if the users can even do that.
javiber
force-pushed
the
before-after-operator
branch
from
95a5579
to
90ef056
Compare
javiber
force-pushed
the
before-after-operator
branch
from
90ef056
to
a406ec6
Compare
Coverage reportMain: 91.15% | PR: 91.15% | Diff: 0.01 ✅ |
1 similar comment
Coverage reportMain: 91.15% | PR: 91.15% | Diff: 0.01 ✅ |
No description provided.