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: round, floor, ceil #349

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

New operators: round, floor, ceil #349

ianspektor opened this issue Jan 17, 2024 · 14 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 for floats: EventSet.round(), .floor(), .ceil().

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

Return same type as received, since floats have a much larger range than ints.

Can possibly be implemented by extending temporian/core/operators/unary.py.

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
@VidhyaVarshanyJS
Copy link

Can I take this one if available?💖

@ianspektor
Copy link
Collaborator Author

Hey @VidhyaVarshanyJS - of course!

This is the first time anyone's contributing a new operator, so please do give any feedback you have on the Developing a new operator guide! And of course I'm here for any questions or help on any design decision 😊

@ianspektor
Copy link
Collaborator Author

Also - we're setting up a Discord to have closer communication with contributors. I'll post the link as soon as it's up!

@VidhyaVarshanyJS
Copy link

Sure!!
I skimmed through the contribution guidelines as well. I'll start the work!
Thank you...

@VidhyaVarshanyJS
Copy link

Hi....
For the float operator ..
You have mentioned to implement them in the unary.py but in the guidelines it mentioned as creating new operators in the init.py or event_set_ops.py file.
Can you clarify this?

@VidhyaVarshanyJS
Copy link

could you provide me with the discord channel?

@ianspektor
Copy link
Collaborator Author

ianspektor commented Jan 29, 2024

Hey!

Yep - implementation can be done in unary.py (see both temporian/core/operators/unary.py and temporian/implementation/numpy/operators/unary.py), since it already implements some logic that can be reused (see that round() and abs(), which is implemented in unary.py already, are very similar operations).

However, you'll also need to add the three new functions in event_set_ops.py to make them available in the EventSet, which call the functions defined in unary.py.

Will create the Discord today/tomorrow and post the link here as soon as it's up!

@VidhyaVarshanyJS
Copy link

Hi...
Here in the event_set_ops.py . Look at line 598 and then at line 1477 why there are two time abs function is defined but for other operator say for example invert the function is defined only at line 558.

Kindly clarify me if I misunderstood .

@VidhyaVarshanyJS
Copy link

I have implemented the round() function . Can I commit that to my branch and make pull request for your review once?

@ianspektor
Copy link
Collaborator Author

abs is defined both as abs() and __abs__() so that it can be called both with EventSet.abs() and abs(EventSet) (defining the abs magic method enables this). __invert__ is only defined as a magic method.

round, floor and ceil all have magic methods with the same name that should be implemented, so round for example needs to be defined both as round() (with a docstring) and __round__() (no docstring needed).

For sure, push the PR and I'll take a look!

@ianspektor
Copy link
Collaborator Author

Discord is up! https://discord.gg/7ySyZ4YE

@VidhyaVarshanyJS
Copy link

I have created the pull request..

@akshatvishu
Copy link
Contributor

Hello there! I'm interested in contributing to open issues related to .floor() and .ceil(). Could you please point me to any available tasks or provide more information about the current status of these issues? I'd love to help out if there's still work to be done! Thank you.

@ianspektor
Copy link
Collaborator Author

Hey @akshatvishu! .floor() and .ceil()'s should be sharing base classes/logic with .round(), which is why they were one same issue (implementing the remaining 2 after implementing the first is super straightforward).

@VidhyaVarshanyJS are you still working on this?

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

3 participants