Fix tests arithm scalar #288
Conversation
DonBraulio
force-pushed
the
fix-tests-arithm-scalar
branch
from
a686763 to
daf38e0
Compare
Coverage reportMain: 90.85% | PR: 90.85% | Diff: 0.00 ✅ |
| @@ -78,11 +83,6 @@ Check the index on the left for a more detailed description of any symbol. | |||
| | [`tp.equal()`][temporian.equal] [`tp.not_equal()`][temporian.not_equal] [`tp.greater()`][temporian.greater] [`tp.greater_equal()`][temporian.greater_equal] [`tp.less()`][temporian.less] [`tp.less_equal()`][temporian.less_equal] | Compute a relational binary operator between two [`EventSets`][temporian.EventSet]. | | |||
There was a problem hiding this comment.
tp.equal() no longer exists right? I'd move all of the binary ones to the EventSet if we're moving .equal(). @achoum is there a reason we haven't already done this?
There was a problem hiding this comment.
I think the main reason is that we already have the magic methods for those. evset.equal() is needed because we can't overload the == operator. But we do have +, *, >=, etc...
IMO we should remove all these from __init__, without adding them to event_set_ops. I don't see any use case where using these methods instead of python ops is a much better option, and we should try to not have two ways to do things.
(this was the last point I wanted to discuss today)
There was a problem hiding this comment.
LGTM from my side to remove them 👍🏼
Coverage reportMain: 90.85% | PR: 90.85% | Diff: 0.00 ✅ |
Coverage reportMain: 90.85% | PR: 90.85% | Diff: 0.00 ✅ |
Moved
tp.equal()toevset.equal(other_evset / scalar_value).Also: Removed unary operators from
__init__(tp.log(),tp.abs(),tp.isnan()), since they were already inevent_set_ops.Tests restructured in this PR:
For a future PR: we should probably remove ops. that are already overloaded from
__init__, such astp.greater_equal(). Unless there a good reason to have 2 ways to do that.