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.

Sign up for GitHub

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

added psi calculation to categorical columns #1027

Merged
merged 5 commits into from
Sep 20, 2023
Merged

added psi calculation to categorical columns #1027

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4792144
added psi calculation to categorical columns
ksneab7 Sep 18, 2023
026b2ef
Changed test value to non-calculated assignment
ksneab7 Sep 18, 2023
86aa39a
Added warning to psi calculation for if it is not calculated
ksneab7 Sep 20, 2023
108d8d8
fix for psi included in column profilers'
ksneab7 Sep 20, 2023
d3d52f7
Changed psi default behavior back to not being included if not able t…
ksneab7 Sep 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions dataprofiler/profilers/categorical_column_profile.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
"""Contains class for categorical column profiler."""
from __future__ import annotations

import math
import warnings
from collections import defaultdict
from operator import itemgetter
from typing import cast
Expand Down Expand Up @@ -304,6 +306,20 @@ 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
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@ksneab7 ksneab7 Sep 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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:

  1. how did the code get called anyway?, if there are no categories the categorical profiler should never be initialized and cant be diffed
  2. Even if we get here the psi of nothing compared to nothing should be zero, psi is used to calculate change between two datasets, if nothing changed because there is nothing in both profiles, returning 0.0 for psi I think as a good thing right?

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
else:
warnings.warn(
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe if we hit this we should just either 1) have PSI to None or 2) not include in the difference ducts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with this if PSI cant be calculated it shouldnt default to zero

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

"psi was not calculated due to the differences in categories "
"of the profiles. Differences:\n"
f"{set(cat_count1.keys()) ^ set(cat_count2.keys())}",
RuntimeWarning,
)

differences["statistics"][
"categorical_count"
Expand Down
41 changes: 39 additions & 2 deletions dataprofiler/tests/profilers/test_categorical_column_profile.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -728,8 +728,13 @@ def test_categorical_diff(self):
},
},
}

self.assertDictEqual(expected_diff, profile.diff(profile2))
with self.assertWarnsRegex(
RuntimeWarning,
"psi was not calculated due to the differences in categories "
"of the profiles. Differences:\n{'maybe'}",
):
test_profile_diff = profile.diff(profile2)
self.assertDictEqual(expected_diff, test_profile_diff)

# Test with one categorical column matching
df_not_categorical = pd.Series(
Expand All @@ -756,6 +761,38 @@ def test_categorical_diff(self):
}
self.assertDictEqual(expected_diff, profile.diff(profile2))

# Test diff with psi enabled
df_categorical = pd.Series(["y", "y", "y", "y", "n", "n", "n", "maybe"])
profile = CategoricalColumn(df_categorical.name)
profile.update(df_categorical)

df_categorical = pd.Series(["y", "maybe", "y", "y", "n", "n", "maybe"])
profile2 = CategoricalColumn(df_categorical.name)
profile2.update(df_categorical)

# chi2-statistic = sum((observed-expected)^2/expected for each category in each column)
# df = categories - 1
# psi = (% of records based on Sample (A) - % of records Sample (B)) * ln(A/ B)
# p-value found through using chi2 CDF
expected_diff = {
"categorical": "unchanged",
"statistics": {
"unique_count": "unchanged",
"unique_ratio": -0.05357142857142855,
"chi2-test": {
"chi2-statistic": 0.6122448979591839,
"df": 2,
Copy link
Contributor

Choose a reason for hiding this comment

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

outside scope of this PR: Does df stand for dataframe here? Looks like it's an int.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No its supposed to an int, its stands for degrees of freedom, but I agree that it is not a great choice of name

"p-value": 0.7362964551863367,
},
"categories": "unchanged",
"gini_impurity": -0.059311224489795866,
"unalikeability": -0.08333333333333326,
"psi": 0.16814961527477595,
"categorical_count": {"y": 1, "n": 1, "maybe": -1},
},
}
self.assertDictEqual(expected_diff, profile.diff(profile2))

def test_unalikeability(self):
df_categorical = pd.Series(["a", "a"])
profile = CategoricalColumn(df_categorical.name)
Expand Down