google / temporian

Temporian is an open-source Python library for preprocessing ⚡ and feature engineering 🛠 temporal data 📈 for machine learning applications 🤖
https://temporian.readthedocs.io
Apache License 2.0
663 stars 43 forks source link

New operators: round, floor, ceil #349

Open ianspektor opened 7 months ago

ianspektor commented 7 months ago

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!

VidhyaVarshanyJS commented 7 months ago

Can I take this one if available?💖

ianspektor commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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

VidhyaVarshanyJS commented 7 months ago

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 commented 7 months ago

could you provide me with the discord channel?

ianspektor commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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

ianspektor commented 7 months ago

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 commented 7 months ago

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

VidhyaVarshanyJS commented 7 months ago

I have created the pull request..

akshatvishu commented 6 months ago

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 commented 6 months ago

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?