added psi calculation to categorical columns #1027
Conversation
ksneab7
requested review from
taylorfturner,
micdavis and
tyfarnan
as code owners
| profile2.update(df_categorical) | ||
|
|
||
| # Calculate expected_psi | ||
| expected_psi = 0 |
There was a problem hiding this comment.
What about other test cases for non zero?
There was a problem hiding this comment.
for perc_A, perc_B in zip(bin_perc, bin_perc_2):
expected_psi += (perc_B - perc_A) * math.log(perc_B / perc_A)
There was a problem hiding this comment.
could we make expected_psi in the expected_diff hardcoded as a value? or would that be tough? I just want the expected to be completely disjoint from any calculations. In this case were using the formula for PSI to check our formula for PSI, sort of...
|
Follow up issue to calculate PSI using the correct functionality that exists within NumericalStatsMixin: |
taylorfturner
added
Bug
| if cat_count1.keys() == cat_count2.keys(): | ||
| total_psi = 0.0 | ||
| for key in cat_count1.keys(): | ||
| perc_A = cat_count1[key] / self.sample_size | ||
| perc_B = cat_count2[key] / other_profile.sample_size | ||
| total_psi += (perc_B - perc_A) * math.log(perc_B / perc_A) | ||
|
|
||
| differences["statistics"]["psi"] = total_psi |
There was a problem hiding this comment.
I think we should raise a warning (or at least post to the logger) that PSI was not calculated and why it was not calculated in this section of the code.
Was looking at L704 in test_categorical_profile.py and that would be a case (L704 - L732) where we that case in L308 is covered but we should assert that none of that code (i.e. a warning or logger) is called
| @@ -304,7 +305,14 @@ def diff(self, other_profile: CategoricalColumn, options: dict = None) -> dict: | |||
| other_profile._categories.items(), key=itemgetter(1), reverse=True | |||
| ) | |||
| ) | |||
|
|
|||
| if cat_count1.keys() == cat_count2.keys(): | |||
| total_psi = 0.0 | |||
There was a problem hiding this comment.
also is there a case where they would equal but the default shouldn't be 0... thinking if .keys() on both is empty (i.e. {}.keys() return dict_keys([])) but the issue is no that on the iter it won't do much but... it will still set psi to 0.0 when should it really? or should we say that is unclculable? add condition for minimum key of len() == 1?
There was a problem hiding this comment.
if the categories are equal and of equal count the psi is zero. So if there are no categories (and by extension no counts so no percentages to calculate) I have a couple questions:
- how did the code get called anyway?, if there are no categories the categorical profiler should never be initialized and cant be diffed
- Even if we get here the
psiof nothing compared to nothing should be zero,psiis used to calculate change between two datasets, if nothing changed because there is nothing in both profiles, returning 0.0 forpsiI think as a good thing right?
| perc_B = cat_count2[key] / other_profile.sample_size | ||
| total_psi += (perc_B - perc_A) * math.log(perc_B / perc_A) | ||
| else: | ||
| warnings.warn( |
There was a problem hiding this comment.
Maybe if we hit this we should just either 1) have PSI to None or 2) not include in the difference ducts
There was a problem hiding this comment.
I agree with this if PSI cant be calculated it shouldnt default to zero
| "unique_ratio": -0.05357142857142855, | ||
| "chi2-test": { | ||
| "chi2-statistic": 0.6122448979591839, | ||
| "df": 2, |
There was a problem hiding this comment.
outside scope of this PR: Does df stand for dataframe here? Looks like it's an int.
There was a problem hiding this comment.
No its supposed to an int, its stands for degrees of freedom, but I agree that it is not a great choice of name
* 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>
No description provided.