Make sure random_state is a list before indexed assignment #968

junholee6a · 2023-07-25T01:23:48Z

Issue: #722

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. This tells mypy that random_state is definitely a list and cannot be a tuple.

taylorfturner · 2023-07-25T14:00:07Z

update branch

JGSweets · 2023-07-27T19:10:22Z

dataprofiler/labelers/data_processing.py

@@ -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)


This might be a case for cast as opposed to a computation. I'm surprised isinstance(random_state, (list, tuple)) isn't alleviating this.

yeah just trick mypy with the cast rather than at execution time actually changing the type to list

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):

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.

@taylorfturner @junholee6a Did this get resolved?

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

taylorfturner

blocking for 0.10.3 release. will unblock post-deployment

dismissing block post 0.10.3

taylorfturner · 2023-08-08T14:02:16Z

will review today. Update branch with dev needed @junholee6a

taylorfturner · 2023-08-09T15:27:37Z

@junholee6a sorry something else merged to dev before this.... can you update branch once more 🙂

The base branch was changed.

taylorfturner · 2023-08-09T15:53:32Z

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

junholee6a · 2023-08-09T16:18:24Z

I can't rebase automatically on github, let me see if I can do so manually

junholee6a · 2023-08-09T16:25:10Z

Rebased

taylorfturner · 2023-08-10T14:33:56Z

Rebased

still showing as out of date with dev 🤔

taylorfturner · 2023-08-28T17:15:24Z

@junholee6a update branch here lol -- @ksneab7 let's review this PR first

dataprofiler/labelers/data_processing.py

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 <[email protected]>

dataprofiler/labelers/data_processing.py

Co-authored-by: Taylor Turner <[email protected]>

…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 <[email protected]> * Reorder conditions for consistency Co-authored-by: Taylor Turner <[email protected]> --------- Co-authored-by: Michael Davis <[email protected]> Co-authored-by: Taylor Turner <[email protected]>

* 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] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Taylor Turner <[email protected]> * 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 <[email protected]> * Reorder conditions for consistency Co-authored-by: Taylor Turner <[email protected]> --------- Co-authored-by: Michael Davis <[email protected]> Co-authored-by: Taylor Turner <[email protected]> * 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] <[email protected]> Co-authored-by: Navid Nafiuzzaman <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Taylor Turner <[email protected]> Co-authored-by: Junho Lee <[email protected]> Co-authored-by: Michael Davis <[email protected]>

junholee6a requested review from JGSweets, ksneab7, taylorfturner, micdavis and tyfarnan as code owners July 25, 2023 01:23

taylorfturner enabled auto-merge (squash) July 25, 2023 11:36

taylorfturner assigned junholee6a Jul 25, 2023

taylorfturner added the static_typing mypy static typing issues label Jul 25, 2023

junholee6a force-pushed the make-list-before-indexed-assignment branch from 8749eb0 to d6b0aa3 Compare July 25, 2023 16:35

junholee6a mentioned this pull request Jul 25, 2023

Delay transforming priority_order into ndarray #969

Closed

tyfarnan previously approved these changes Jul 27, 2023

View reviewed changes

JGSweets reviewed Jul 27, 2023

View reviewed changes

ksneab7 previously approved these changes Aug 7, 2023

View reviewed changes

taylorfturner previously requested changes Aug 7, 2023

View reviewed changes

junholee6a force-pushed the make-list-before-indexed-assignment branch from d6b0aa3 to 32c414b Compare August 8, 2023 15:17

auto-merge was automatically disabled August 8, 2023 15:17
Head branch was pushed to by a user without write access

taylorfturner changed the base branch from old-dev to dev August 9, 2023 15:37

junholee6a force-pushed the make-list-before-indexed-assignment branch from 0f361ec to ae2e834 Compare August 9, 2023 16:23

junholee6a force-pushed the make-list-before-indexed-assignment branch from ae2e834 to 9c02d41 Compare August 10, 2023 16:02

taylorfturner enabled auto-merge (squash) August 10, 2023 16:43

auto-merge was automatically disabled September 1, 2023 21:41
Head branch was pushed to by a user without write access

junholee6a force-pushed the make-list-before-indexed-assignment branch from 2a63511 to 555b0a7 Compare September 1, 2023 21:41

taylorfturner enabled auto-merge (squash) September 5, 2023 16:04

taylorfturner previously approved these changes Sep 6, 2023

View reviewed changes

micdavis reviewed Sep 8, 2023

View reviewed changes

dataprofiler/labelers/data_processing.py Outdated Show resolved Hide resolved

auto-merge was automatically disabled September 8, 2023 15:17
Head branch was pushed to by a user without write access

junholee6a dismissed taylorfturner’s stale review via 91b12ab September 8, 2023 15:17

junholee6a and others added 3 commits September 8, 2023 11:40

Add type guards for random_state

1578c33

Check random_state before random_state[1]

ec031d9

Co-authored-by: Michael Davis <[email protected]>

junholee6a force-pushed the make-list-before-indexed-assignment branch from 91b12ab to ec031d9 Compare September 8, 2023 15:40

taylorfturner enabled auto-merge (squash) September 11, 2023 19:41

taylorfturner requested review from ksneab7 and micdavis September 12, 2023 12:52

taylorfturner suggested changes Sep 12, 2023

View reviewed changes

dataprofiler/labelers/data_processing.py Outdated Show resolved Hide resolved

Reorder conditions for consistency

5d70c6b

Co-authored-by: Taylor Turner <[email protected]>

auto-merge was automatically disabled September 12, 2023 14:16
Head branch was pushed to by a user without write access

taylorfturner enabled auto-merge (squash) September 12, 2023 14:25

taylorfturner approved these changes Sep 12, 2023

View reviewed changes

ksneab7 approved these changes Sep 13, 2023

View reviewed changes

taylorfturner merged commit ff14ab3 into capitalone:dev Sep 13, 2023
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make sure random_state is a list before indexed assignment #968

Make sure random_state is a list before indexed assignment #968

junholee6a commented Jul 25, 2023 •

edited

Loading

taylorfturner commented Jul 25, 2023

JGSweets Jul 27, 2023

taylorfturner Jul 27, 2023

junholee6a Jul 27, 2023 •

edited

Loading

junholee6a Jul 27, 2023

ksneab7 Aug 28, 2023 •

edited

Loading

junholee6a Sep 1, 2023

taylorfturner left a comment

taylorfturner commented Aug 8, 2023

taylorfturner commented Aug 9, 2023

taylorfturner commented Aug 9, 2023

junholee6a commented Aug 9, 2023

junholee6a commented Aug 9, 2023

taylorfturner commented Aug 10, 2023

taylorfturner commented Aug 28, 2023

Make sure random_state is a list before indexed assignment #968

Make sure random_state is a list before indexed assignment #968

Conversation

junholee6a commented Jul 25, 2023 • edited Loading

taylorfturner commented Jul 25, 2023

JGSweets Jul 27, 2023

Choose a reason for hiding this comment

taylorfturner Jul 27, 2023

Choose a reason for hiding this comment

junholee6a Jul 27, 2023 • edited Loading

Choose a reason for hiding this comment

junholee6a Jul 27, 2023

Choose a reason for hiding this comment

ksneab7 Aug 28, 2023 • edited Loading

Choose a reason for hiding this comment

junholee6a Sep 1, 2023

Choose a reason for hiding this comment

taylorfturner left a comment

Choose a reason for hiding this comment

taylorfturner commented Aug 8, 2023

taylorfturner commented Aug 9, 2023

taylorfturner commented Aug 9, 2023

junholee6a commented Aug 9, 2023

junholee6a commented Aug 9, 2023

taylorfturner commented Aug 10, 2023

taylorfturner commented Aug 28, 2023

junholee6a commented Jul 25, 2023 •

edited

Loading

junholee6a Jul 27, 2023 •

edited

Loading

ksneab7 Aug 28, 2023 •

edited

Loading