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

New operators: string operations #347

Open
ianspektor opened this issue Jan 17, 2024 · 7 comments
Open

New operators: string operations #347

ianspektor opened this issue Jan 17, 2024 · 7 comments
Assignees
Labels
good first issue Good for newcomers new operator Development of a new operator.

Comments

@ianspektor
Copy link
Collaborator

ianspektor commented Jan 17, 2024

New operators to operate on strings:

  • add
  • multiply
  • lower
  • upper
  • str_len

Implementation should be factored with base classes so that each operation only needs to define the np.char method it needs to call.

See https://github.com/google/temporian/blob/main/CONTRIBUTING.md#developing-a-new-operator for guidance.

Questions or requests for additional guidance from possible contributors more than welcome!

@ianspektor ianspektor added good first issue Good for newcomers new operator Development of a new operator. labels Jan 17, 2024
@tanaysd
Copy link
Contributor

tanaysd commented Feb 14, 2024

@ianspektor I will take this next, can you please assign this to me?

@ianspektor
Copy link
Collaborator Author

@tanaysd for sure! Thanks :)

Some notes:

  • Read and pay close attention to the operator development guide (link in the issue's description)
  • Check how logical operators (i.e. and, or and xor) are implemented for reference, since those only accept and return bool types, which is similar to these that accept and return strings
    • Key files: temporian/core/operators/binary/logical.py, temporian/implementation/numpy/operators/binary/logical.py, temporian/core/operators/test/test_logical.py
    • Although note that these string ops shouldn't modify the feature names they return (logical ops do because they receive 2 EventSets)
    • Please make tests a bit more thorough than the ones in test_logical, which only test success cases (e.g. test that the expected error is returned when passing an incorrect type)
  • Make sure you add the new operators to the docs (as described in the guide) and you can see them when running the docs locally

@ianspektor
Copy link
Collaborator Author

ianspektor commented Feb 15, 2024

Also, let's start out with just 2 of the operations in the list (let's say upper() and lower()) and only implement the remaining ones after we've validated the design, so you don't need to be making changes to all 9 on every comment, if that sounds alright

@tanaysd
Copy link
Contributor

tanaysd commented Feb 15, 2024

Sounds good, thank you, can you share the link to discord server when you've a chance?

@ianspektor
Copy link
Collaborator Author

https://discord.gg/nT54yATCTy 💪🏼

@tanaysd
Copy link
Contributor

tanaysd commented Feb 25, 2024

Hi @ianspektor

  1. should string operators get their own folder similar to calendar

found this snippet at the end of the instructions Groups of operator with a similar implementation as grouped together. For instance, temporian/core/operators/window contains moving window operators

  1. in your last comment you mention 9 but the original description lists five operators, which one is correct?

@ianspektor
Copy link
Collaborator Author

It's 5, sorry for the confusion, there were some other ones that I moved to this issue since they need to be extended from existing functions to work with strings (vs. created from scratch like these): #369

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers new operator Development of a new operator.
Projects
None yet
Development

No branches or pull requests

2 participants