New operators: round, floor, ceil
Opened this issue · 14 comments
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!
Can I take this one if available?💖
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 😊
Also - we're setting up a Discord to have closer communication with contributors. I'll post the link as soon as it's up!
Sure!!
I skimmed through the contribution guidelines as well. I'll start the work!
Thank you...
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?
could you provide me with the discord channel?
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!
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 .
I have implemented the round() function . Can I commit that to my branch and make pull request for your review once?
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!
Discord is up! https://discord.gg/7ySyZ4YE
I have created the pull request..
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.
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?