Skip to content
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

[REVIEW] Fix operator NotImplemented issue with numpy #11816

Merged
merged 3 commits into from
Sep 29, 2022

Conversation

galipremsagar
Copy link
Contributor

Description

Root cause:

In [1]: import numpy as np

In [2]: x = np.uint8(1)

In [3]: y = np.float64(1.0)

In [4]: x.__ge__(y)
Out[4]: NotImplemented

In [8]: x >= y
Out[8]: True

This is leading to the following error whenever there is a Scalar binary operation involved:

python/cudf/cudf/tests/test_series.py:449: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
../envs/cudfdev/lib/python3.9/contextlib.py:79: in inner
    return func(*args, **kwds)
../envs/cudfdev/lib/python3.9/site-packages/cudf/core/series.py:2988: in describe
    data = _describe_categorical(self, percentiles)
../envs/cudfdev/lib/python3.9/site-packages/cudf/core/series.py:152: in _describe_categorical
    val_counts = obj.value_counts(ascending=False)
../envs/cudfdev/lib/python3.9/contextlib.py:79: in inner
    return func(*args, **kwds)
../envs/cudfdev/lib/python3.9/site-packages/cudf/core/series.py:2862: in value_counts
    res = res.sort_values(ascending=ascending)
../envs/cudfdev/lib/python3.9/contextlib.py:79: in inner
    return func(*args, **kwds)
../envs/cudfdev/lib/python3.9/site-packages/cudf/core/series.py:1910: in sort_values
    return super().sort_values(
../envs/cudfdev/lib/python3.9/site-packages/cudf/core/indexed_frame.py:1916: in sort_values
    out = self._gather(
../envs/cudfdev/lib/python3.9/site-packages/cudf/core/indexed_frame.py:1523: in _gather
    if not libcudf.copying._gather_map_is_valid(
copying.pyx:67: in cudf._lib.copying._gather_map_is_valid
    ???
../envs/cudfdev/lib/python3.9/site-packages/cudf/core/mixins/mixin_factory.py:11: in wrapper
    return method(self, *args1, *args2, **kwargs1, **kwargs2)
../envs/cudfdev/lib/python3.9/site-packages/cudf/core/scalar.py:350: in _binaryop
    return Scalar(result, dtype=out_dtype)
../envs/cudfdev/lib/python3.9/site-packages/cudf/core/scalar.py:56: in __call__
    obj = super().__call__(value, dtype=dtype)
../envs/cudfdev/lib/python3.9/site-packages/cudf/core/scalar.py:128: in __init__
    self._host_value, self._host_dtype = self._preprocess_host_value(
../envs/cudfdev/lib/python3.9/site-packages/cudf/core/scalar.py:222: in _preprocess_host_value
    value = to_cudf_compatible_scalar(value, dtype=dtype)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

val = NotImplemented, dtype = <class 'numpy.bool_'>

    def to_cudf_compatible_scalar(val, dtype=None):
        """
        Converts the value `val` to a numpy/Pandas scalar,
        optionally casting to `dtype`.
    
        If `val` is None, returns None.
        """
    
        if cudf._lib.scalar._is_null_host_scalar(val) or isinstance(
            val, cudf.Scalar
        ):
            return val
    
        if not cudf.api.types._is_scalar_or_zero_d_array(val):
>           raise ValueError(
                f"Cannot convert value of type {type(val).__name__} "
                "to cudf scalar"
            )
E           ValueError: Cannot convert value of type NotImplementedType to cudf scalar

../envs/cudfdev/lib/python3.9/site-packages/cudf/utils/dtypes.py:248: ValueError

This PR fixes the issue by first trying to call the op with operator standard library and then try to getattr if the op is not found in operator module.

Checklist

@galipremsagar galipremsagar added bug Something isn't working 3 - Ready for Review Ready for review by team Python Affects Python cuDF API. 4 - Needs cuDF (Python) Reviewer non-breaking Non-breaking change labels Sep 29, 2022
@galipremsagar galipremsagar self-assigned this Sep 29, 2022
@galipremsagar galipremsagar added this to PR-WIP in v22.10 Release via automation Sep 29, 2022
@galipremsagar galipremsagar requested a review from a team as a code owner September 29, 2022 02:14
@galipremsagar galipremsagar moved this from PR-WIP to PR-Needs review in v22.10 Release Sep 29, 2022
@codecov
Copy link

codecov bot commented Sep 29, 2022

Codecov Report

❗ No coverage uploaded for pull request base (branch-22.10@5a4afec). Click here to learn what that means.
Patch has no changes to coverable lines.

Additional details and impacted files
@@               Coverage Diff               @@
##             branch-22.10   #11816   +/-   ##
===============================================
  Coverage                ?   87.51%           
===============================================
  Files                   ?      133           
  Lines                   ?    21807           
  Branches                ?        0           
===============================================
  Hits                    ?    19085           
  Misses                  ?     2722           
  Partials                ?        0           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Contributor

@wence- wence- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a test, for example:

diff --git a/python/cudf/cudf/tests/test_scalar.py b/python/cudf/cudf/tests/test_scalar.py
index 90268ebabe..1d7c09c70d 100644
--- a/python/cudf/cudf/tests/test_scalar.py
+++ b/python/cudf/cudf/tests/test_scalar.py
@@ -412,6 +412,13 @@ def test_datetime_scalar_from_string(data, dtype):
     assert expected == slr.value
 
 
+def test_scalar_numpy_casting():
+    # binop should upcast to wider type
+    s1 = cudf.Scalar(1, dtype=np.int32)
+    s2 = np.int64(2)
+    assert s1 < s2
+
+
 def test_scalar_cache():
     s = cudf.Scalar(1)
     s2 = cudf.Scalar(1)

python/cudf/cudf/core/scalar.py Outdated Show resolved Hide resolved
v22.10 Release automation moved this from PR-Needs review to PR-Reviewer approved Sep 29, 2022
@galipremsagar
Copy link
Contributor Author

+def test_scalar_numpy_casting():
+    # binop should upcast to wider type
+    s1 = cudf.Scalar(1, dtype=np.int32)
+    s2 = np.int64(2)
+    assert s1 < s2

Done 👍

@galipremsagar galipremsagar added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 3 - Ready for Review Ready for review by team 4 - Needs cuDF (Python) Reviewer labels Sep 29, 2022
@galipremsagar
Copy link
Contributor Author

@gpucibot merge

@rapids-bot rapids-bot bot merged commit ddfd07f into rapidsai:branch-22.10 Sep 29, 2022
v22.10 Release automation moved this from PR-Reviewer approved to Done Sep 29, 2022
@galipremsagar galipremsagar mentioned this pull request Sep 29, 2022
3 tasks
raydouglass pushed a commit that referenced this pull request Sep 29, 2022
## Description
Because of the issue discovered here: #11816, we would like to pin the max version of `numpy` to `<1.23` thus averting the same error in `22.08`. 

## Checklist
- [x] I am familiar with the [Contributing Guidelines](https://github.com/rapidsai/cudf/blob/HEAD/CONTRIBUTING.md).
- [x] New or existing tests cover these changes.
- [x] The documentation is up to date with these changes.

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

Approvers:
   - Ray Douglass (https://github.com/raydouglass)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - Ready to Merge Testing and reviews complete, ready to merge bug Something isn't working non-breaking Non-breaking change Python Affects Python cuDF API.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants