-
Notifications
You must be signed in to change notification settings - Fork 164
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
Make sure random_state is a list before indexed assignment #968
Make sure random_state is a list before indexed assignment #968
Conversation
update branch |
8749eb0
to
d6b0aa3
Compare
@@ -1556,7 +1556,8 @@ def __init__( | |||
elif isinstance(random_state, (list, tuple)) and len(random_state) == 3: | |||
# tuple required for random state to be set, lists do not work | |||
if isinstance(random_state[1], list): | |||
random_state[1] = tuple(random_state[1]) # type: ignore | |||
random_state = list(random_state) |
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.
This might be a case for cast as opposed to a computation. I'm surprised isinstance(random_state, (list, tuple))
isn't alleviating this.
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.
yeah just trick mypy
with the cast rather than at execution time actually changing the type to list
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.
This might be a case for cast as opposed to a computation. I'm surprised isinstance(random_state, (list, tuple)) isn't alleviating this.
It's because random_state
could be a tuple, in which case indexed assignment isn't possible.
Is there any possibility that random_state
is a tuple while random_state[1]
is a list? If this will never happen we can safely cast here, or change the line 1558 conditional to if isinstance(random_state[1], list) and isinstance(random_state, list):
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.
Is there any possibility that random_state is a tuple while random_state[1] is a list?
Though, if this ever did happen in the past it would've thrown a runtime error.
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.
@taylorfturner @junholee6a Did this get resolved?
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.
or change the line 1558 conditional to if isinstance(random_state[1], list) and isinstance(random_state, list):
I just committed this solution since it avoids casting
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.
blocking for 0.10.3 release. will unblock post-deployment
will review today. Update branch with |
d6b0aa3
to
32c414b
Compare
Head branch was pushed to by a user without write access
@junholee6a sorry something else merged to dev before this.... can you update branch once more 🙂 |
The base branch was changed.
You might need to rebase... looking at the commits in this PR: some of them aren't probably related to the work you did on this PR itself |
I can't rebase automatically on github, let me see if I can do so manually |
0f361ec
to
ae2e834
Compare
Rebased |
still showing as out of date with |
ae2e834
to
9c02d41
Compare
@junholee6a update branch here lol -- @ksneab7 let's review this PR first |
Head branch was pushed to by a user without write access
2a63511
to
555b0a7
Compare
Head branch was pushed to by a user without write access
Currently, a mypy error occurs because we attempt to assign to random_state[1] when random_state has type Union[list[Any], tuple[Any]]. Tuples are immutable so this is a type error. We fix this by making random_state into a list before doing indexed assignment on it.
Co-authored-by: Michael Davis <36012613+micdavis@users.noreply.github.com>
91b12ab
to
ec031d9
Compare
Co-authored-by: Taylor Turner <taylorfturner@gmail.com>
Head branch was pushed to by a user without write access
…e#968) * Make sure random_state is a list before indexed assignment Currently, a mypy error occurs because we attempt to assign to random_state[1] when random_state has type Union[list[Any], tuple[Any]]. Tuples are immutable so this is a type error. We fix this by making random_state into a list before doing indexed assignment on it. * Add type guards for random_state * Check random_state before random_state[1] Co-authored-by: Michael Davis <36012613+micdavis@users.noreply.github.com> * Reorder conditions for consistency Co-authored-by: Taylor Turner <taylorfturner@gmail.com> --------- Co-authored-by: Michael Davis <36012613+micdavis@users.noreply.github.com> Co-authored-by: Taylor Turner <taylorfturner@gmail.com>
* modified the assignees for issue creation (#1016) * Minor: Profiler Path Fix in Example Notebook (#1021) * Bump actions/checkout from 3 to 4 (#1024) Bumps [actions/checkout](https://github.com/actions/checkout) from 3 to 4. - [Release notes](https://github.com/actions/checkout/releases) - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md) - [Commits](actions/checkout@v3...v4) --- updated-dependencies: - dependency-name: actions/checkout dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Taylor Turner <taylorfturner@gmail.com> * Make sure random_state is a list before indexed assignment (#968) * Make sure random_state is a list before indexed assignment Currently, a mypy error occurs because we attempt to assign to random_state[1] when random_state has type Union[list[Any], tuple[Any]]. Tuples are immutable so this is a type error. We fix this by making random_state into a list before doing indexed assignment on it. * Add type guards for random_state * Check random_state before random_state[1] Co-authored-by: Michael Davis <36012613+micdavis@users.noreply.github.com> * Reorder conditions for consistency Co-authored-by: Taylor Turner <taylorfturner@gmail.com> --------- Co-authored-by: Michael Davis <36012613+micdavis@users.noreply.github.com> Co-authored-by: Taylor Turner <taylorfturner@gmail.com> * added psi calculation to categorical columns (#1027) * added psi calculation to categorical columns * Changed test value to non-calculated assignment --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: Navid Nafiuzzaman <mxn4459@rit.edu> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Taylor Turner <taylorfturner@gmail.com> Co-authored-by: Junho Lee <53921230+junholee6a@users.noreply.github.com> Co-authored-by: Michael Davis <36012613+micdavis@users.noreply.github.com>
Issue: #722
Currently, a mypy error occurs because we attempt to assign to
random_state[1]
whenrandom_state
has typeUnion[list[Any], tuple[Any]]
. Tuples are immutable so this is a type error.We fix this by making
random_state
into a list before doing indexed assignment on it. This tells mypy thatrandom_state
is definitely alist
and cannot be atuple
.