google / google/temporian

Extend EventSet's >, <, >=, <= to work with string literals

Open
#369 2 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

bug good first issue
Dominant language
Python
Stars
713
Forks
48
PR merge metrics
No merged PRs in 30d

Description

  • EventSet < EventSet works with two numerical EventSets
  • EventSet < int works with a numerical EventSet
  • EventSet < EventSet works with two string EventSets
  • EventSet < str doesn't work with a string EventSet, and it should:
In [1]: import temporian as tp

In [2]: evset = tp.event_set([1], features={'a': [3]})

In [3]: evset < 4
Out[3]:
indexes: []
features: [('a', bool_)]
events:
     (1 events):
        timestamps: [1.]
        'a': [ True]
memory usage: 0.5 kB

In [4]: evset = tp.event_set([1], features={'a': ['a']})

In [5]: evset < 'b'
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
Cell In[8], line 1
----> 1 evset < 'b'

File ~/temporian/temporian/core/event_set_ops.py:1233, in EventSetOperations.__lt__(self, other)
   1229     from temporian.core.operators.scalar import less_scalar
   1231     return less_scalar(input=self, value=other)
-> 1233 self._raise_error("compare", other, "(int,float)")
   1234 assert False

File ~/temporian/temporian/core/event_set_ops.py:88, in EventSetOperations._raise_error(self, op_name, other, allowed_types)
     80 def _raise_error(
     81     self, op_name: str, other: Any, allowed_types: str
     82 ) -> None:
     83     """Raises an error message.
     84
     85     This utility method is used in operator implementations, e.g., +, - *.
     86     """
---> 88     raise ValueError(
     89         f"Cannot use operator '{op_name}' on {self._clsname} and"
     90         f" {type(other)} objects. Only {self._clsname} or values of type"
     91         f" ({allowed_types}) are supported."
     92     )

ValueError: Cannot use operator 'compare' on EventSet and <class 'str'> objects. Only EventSet or values of type ((int,float)) are supported.

Same goes for all other comparison ops.

The T_SCALAR tuple defined in temporian/core/event_set_ops.py only accounts for (int, float), but in some ops it should include str too. A second tuple needs to be defined for it and used where appropiate.

Note that equal and __ne__ in the same file use T_SCALAR + (bool, str), which could be defined as a third option.

Thorough tests needed to ensure we are accepting and rejecting the correct type on each op.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in temporian/core/event_set_ops.py by reviewing T_SCALAR, the comparison operator implementations, and the existing equal and ne handling. Check how scalar types are accepted for each comparison, then add thorough tests covering supported and rejected types for all four operators; done means string literals work where intended without accepting invalid types.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
data
Issue type
Feature
Difficulty
3/5
Estimated time
1-2 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
72/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.