-
Notifications
You must be signed in to change notification settings - Fork 159
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
[FEAT] Implements trigonometry expressions: arctanh arccosh arcsinh #2476
[FEAT] Implements trigonometry expressions: arctanh arccosh arcsinh #2476
Conversation
requesting review @colin-ho |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this @fedemagnani ! Just 2 small nits and this PR should be good to go
tests/table/test_eval.py
Outdated
@@ -367,6 +367,51 @@ def test_table_numeric_atan2_literals() -> None: | |||
) | |||
|
|||
|
|||
def test_table_numeric_arctanh() -> None: | |||
table = MicroPartition.from_pydict({"a": [0.0, 0.5, 0.9, -0.9, -0.5, -0.0, math.nan]}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also test that for values |x| >= 1
, the arctanh expression returns Nan
?
tests/table/test_eval.py
Outdated
|
||
|
||
def test_table_numeric_arccosh() -> None: | ||
table = MicroPartition.from_pydict({"a": [1.0, 2.0, 1.5, math.nan]}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And here, if you could also test that values x < 1
evaluate to NaN
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2476 +/- ##
=======================================
Coverage ? 63.43%
=======================================
Files ? 947
Lines ? 106377
Branches ? 0
=======================================
Hits ? 67479
Misses ? 38898
Partials ? 0
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thanks for this contribution @fedemagnani !
Closes #2470
Closes #2471
Closes #2472