Skip to content

Commit

Permalink
Fix decimal binary operations (#12142)
Browse files Browse the repository at this point in the history 
Fixes: #11636 
This PR:

- [x] Fixes an `UnboundLocalError` error.
- [x] Fixes reflected binary operations and added tests for the same.

Authors:
  - GALI PREM SAGAR (https://github.com/galipremsagar)

Approvers:
  - Bradley Dice (https://github.com/bdice)

URL: #12142
  • Loading branch information
@galipremsagar
galipremsagar authored Nov 16, 2022
1 parent 90f0a77 commit c574ddf
Show file tree
Hide file tree
Showing 3 changed files with 115 additions and 16 deletions.
5 changes: 1 addition & 4 deletions python/cudf/cudf/_lib/binaryop.pyx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ from cudf._lib.cpp.scalar.scalar cimport scalar
from cudf._lib.cpp.types cimport data_type, type_id
from cudf._lib.types cimport dtype_to_data_type, underlying_type_t_type_id

from cudf.api.types import is_scalar, is_string_dtype
from cudf.api.types import is_scalar

cimport cudf._lib.cpp.binaryop as cpp_binaryop
from cudf._lib.cpp.binaryop cimport binary_operator
Expand Down Expand Up @@ -173,7 +173,6 @@ def binaryop(lhs, rhs, op, dtype):
cdef data_type c_dtype = dtype_to_data_type(dtype)

if is_scalar(lhs) or lhs is None:
is_string_col = is_string_dtype(rhs.dtype)
s_lhs = as_device_scalar(lhs, dtype=rhs.dtype if lhs is None else None)
result = binaryop_s_v(
s_lhs,
Expand All @@ -183,7 +182,6 @@ def binaryop(lhs, rhs, op, dtype):
)

elif is_scalar(rhs) or rhs is None:
is_string_col = is_string_dtype(lhs.dtype)
s_rhs = as_device_scalar(rhs, dtype=lhs.dtype if rhs is None else None)
result = binaryop_v_s(
lhs,
Expand All @@ -193,7 +191,6 @@ def binaryop(lhs, rhs, op, dtype):
)

else:
is_string_col = is_string_dtype(lhs.dtype)
result = binaryop_v_v(
lhs,
rhs,
Expand Down
18 changes: 10 additions & 8 deletions python/cudf/cudf/core/column/decimal.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,12 +79,14 @@ def _binaryop(self, other: ColumnBinaryOperand, op: str):

# Binary Arithmetics between decimal columns. `Scale` and `precision`
# are computed outside of libcudf
unsupported_msg = (
f"{op} not supported for the following dtypes: "
f"{self.dtype}, {other.dtype}"
)
try:
if op in {"__add__", "__sub__", "__mul__", "__div__"}:
output_type = _get_decimal_type(self.dtype, other.dtype, op)
result = libcudf.binaryop.binaryop(
self, other, op, output_type
)
output_type = _get_decimal_type(lhs.dtype, rhs.dtype, op)
result = libcudf.binaryop.binaryop(lhs, rhs, op, output_type)
# TODO: Why is this necessary? Why isn't the result's
# precision already set correctly based on output_type?
result.dtype.precision = output_type.precision
Expand All @@ -96,12 +98,12 @@ def _binaryop(self, other: ColumnBinaryOperand, op: str):
"__le__",
"__ge__",
}:
result = libcudf.binaryop.binaryop(self, other, op, bool)
result = libcudf.binaryop.binaryop(lhs, rhs, op, bool)
else:
raise TypeError(unsupported_msg)
except RuntimeError as e:
if "Unsupported operator for these types" in str(e):
raise NotImplementedError(
f"{op} not supported for types with different bit-widths"
) from e
raise TypeError(unsupported_msg) from e
raise

return result
Expand Down
108 changes: 104 additions & 4 deletions python/cudf/cudf/tests/test_binops.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1775,7 +1775,7 @@ def test_binops_with_NA_consistent(dtype, op):


@pytest.mark.parametrize(
"args",
"op, lhs, l_dtype, rhs, r_dtype, expect, expect_dtype",
[
(
operator.add,
Expand All @@ -1786,6 +1786,15 @@ def test_binops_with_NA_consistent(dtype, op):
["3.0", "4.0"],
cudf.Decimal64Dtype(scale=2, precision=4),
),
(
operator.add,
2,
cudf.Decimal64Dtype(scale=2, precision=3),
["1.5", "2.0"],
cudf.Decimal64Dtype(scale=2, precision=3),
["3.5", "4.0"],
cudf.Decimal64Dtype(scale=2, precision=4),
),
(
operator.add,
["1.5", "2.0"],
Expand Down Expand Up @@ -1831,6 +1840,15 @@ def test_binops_with_NA_consistent(dtype, op):
["99.9", "199.8"],
cudf.Decimal128Dtype(scale=6, precision=19),
),
(
operator.sub,
2,
cudf.Decimal64Dtype(scale=3, precision=4),
["2.25", "1.005"],
cudf.Decimal64Dtype(scale=3, precision=4),
["-0.25", "0.995"],
cudf.Decimal64Dtype(scale=3, precision=5),
),
(
operator.mul,
["1.5", "2.0"],
Expand Down Expand Up @@ -1858,6 +1876,15 @@ def test_binops_with_NA_consistent(dtype, op):
["343.0", "1000.0"],
cudf.Decimal64Dtype(scale=0, precision=8),
),
(
operator.mul,
200,
cudf.Decimal64Dtype(scale=3, precision=6),
["0.343", "0.500"],
cudf.Decimal64Dtype(scale=3, precision=6),
["68.60", "100.0"],
cudf.Decimal64Dtype(scale=6, precision=13),
),
(
operator.truediv,
["1.5", "2.0"],
Expand Down Expand Up @@ -1885,6 +1912,15 @@ def test_binops_with_NA_consistent(dtype, op):
["56.77", "1.79"],
cudf.Decimal128Dtype(scale=13, precision=25),
),
(
operator.truediv,
20,
cudf.Decimal128Dtype(scale=2, precision=6),
["20", "20"],
cudf.Decimal128Dtype(scale=2, precision=6),
["1.0", "1.0"],
cudf.Decimal128Dtype(scale=9, precision=15),
),
(
operator.add,
["1.5", None, "2.0"],
Expand Down Expand Up @@ -2103,10 +2139,12 @@ def test_binops_with_NA_consistent(dtype, op):
),
],
)
def test_binops_decimal(args):
op, lhs, l_dtype, rhs, r_dtype, expect, expect_dtype = args
def test_binops_decimal(op, lhs, l_dtype, rhs, r_dtype, expect, expect_dtype):

a = utils._decimal_series(lhs, l_dtype)
if isinstance(lhs, (int, float)):
a = cudf.Scalar(lhs, l_dtype)
else:
a = utils._decimal_series(lhs, l_dtype)
b = utils._decimal_series(rhs, r_dtype)
expect = (
utils._decimal_series(expect, expect_dtype)
Expand All @@ -2122,6 +2160,68 @@ def test_binops_decimal(args):
utils.assert_eq(expect, got)


@pytest.mark.parametrize(
"op,lhs,l_dtype,rhs,r_dtype,expect,expect_dtype",
[
(
"radd",
["1.5", "2.0"],
cudf.Decimal64Dtype(scale=2, precision=3),
["1.5", "2.0"],
cudf.Decimal64Dtype(scale=2, precision=3),
["3.0", "4.0"],
cudf.Decimal64Dtype(scale=2, precision=4),
),
(
"rsub",
["100", "200"],
cudf.Decimal64Dtype(scale=-2, precision=10),
["0.1", "0.2"],
cudf.Decimal64Dtype(scale=6, precision=10),
["-99.9", "-199.8"],
cudf.Decimal128Dtype(scale=6, precision=19),
),
(
"rmul",
["1000", "2000"],
cudf.Decimal64Dtype(scale=-3, precision=4),
["0.343", "0.500"],
cudf.Decimal64Dtype(scale=3, precision=3),
["343.0", "1000.0"],
cudf.Decimal64Dtype(scale=0, precision=8),
),
(
"rtruediv",
["1.5", "0.5"],
cudf.Decimal64Dtype(scale=3, precision=6),
["1.5", "2.0"],
cudf.Decimal64Dtype(scale=3, precision=6),
["1.0", "4.0"],
cudf.Decimal64Dtype(scale=10, precision=16),
),
],
)
def test_binops_reflect_decimal(
op, lhs, l_dtype, rhs, r_dtype, expect, expect_dtype
):

a = utils._decimal_series(lhs, l_dtype)
b = utils._decimal_series(rhs, r_dtype)
expect = utils._decimal_series(expect, expect_dtype)

got = getattr(a, op)(b)
assert expect.dtype == got.dtype
utils.assert_eq(expect, got)


def test_binops_raise_error():
s = cudf.Series([decimal.Decimal("1.324324")])
with pytest.raises(TypeError):
s**1
with pytest.raises(TypeError):
s // 1


@pytest.mark.parametrize(
"args",
[
Expand Down

0 comments on commit c574ddf

Please sign in to comment.