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

Fix check on RandomState type to use isinstance #3644

Closed
wants to merge 2 commits into from

Conversation

frankier
Copy link

Currently if we try and pass in a RandomState to e.g. make_classification(...) like so:

from cuml.datasets import make_classification
from cupy.random import RandomState
rng = RandomState(42)
x, y = make_classification(40, 10, random_state=rng)

then we get:

  File "/opt/conda/lib/python3.8/site-packages/cuml/internals/api_decorators.py", line 473, in inner
    ret_val = func(*args, **kwargs)
  File "/opt/conda/lib/python3.8/site-packages/cuml/datasets/classification.py", line 210, in make_classification
    generator = _create_rs_generator(random_state)
  File "/opt/conda/lib/python3.8/site-packages/cuml/datasets/utils.py", line 39, in _create_rs_generator
    raise ValueError('random_state type must be int or CuPy RandomState')
ValueError: random_state type must be int or CuPy RandomState

This PR replaces the current type detection approach with a more conventional isinstance(...) check, fixing the problem.

@frankier frankier requested a review from a team as a code owner March 22, 2021 13:16
@GPUtester
Copy link
Contributor

Can one of the admins verify this patch?

@github-actions github-actions bot added the Cython / Python Cython or Python issue label Mar 22, 2021
@JohnZed
Copy link
Contributor

JohnZed commented Mar 22, 2021

ok to test

@@ -25,16 +25,9 @@ def _create_rs_generator(random_state):
The random_state from which the CuPy random state is generated
Copy link
Contributor

Choose a reason for hiding this comment

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

You'll need to update the copyright header to include 2021 for CI to run here.

@JohnZed JohnZed added the 4 - Waiting on Author Waiting for author to respond to review label Mar 24, 2021
@JohnZed JohnZed added bug Something isn't working non-breaking Non-breaking change labels Mar 31, 2021
@JohnZed
Copy link
Contributor

JohnZed commented Mar 31, 2021

rerun tests

@JohnZed
Copy link
Contributor

JohnZed commented Apr 6, 2021

Thank you for this implementation! Ended up adding a slightly-different version of your same idea in #3716

@JohnZed JohnZed closed this Apr 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4 - Waiting on Author Waiting for author to respond to review bug Something isn't working Cython / Python Cython or Python issue non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants