Adding `PSI` to `diff` report #688

taylorfturner · 2022-10-13T15:01:24Z

Adding PSI to diff report as part of issue #633

adding PSI def _calculate_psi() and implement in NumericStatsMixin.diff()
updated unit testing to reflect psi output as part of diff()

taylorfturner · 2022-10-13T15:32:18Z

dataprofiler/profilers/numerical_column_stats.py

@@ -349,6 +349,8 @@ def report(self, remove_disabled_flag: bool = False) -> Dict:
                    continue
                elif profile_key == "times":
                    continue
+                elif profile_key == "psi":


add option for disabling flag psi

can we combine this with times and create a in [...]?

we could... I kinda see times and psi as two distinct keys ... whereas stddev and variance together make sense becuase they are derivatives of each other

they are different, but the logic is the same. if the key is this, continue. whereas stddev / variance are dependent hence, if one is not there, we have to pop the other.

ah I see what you're getting at ... fair critique of the code. I'm game for a elif profile_key in ['psi', 'times']

taylorfturner · 2022-10-13T15:32:36Z

dataprofiler/profilers/numerical_column_stats.py

@@ -391,6 +393,7 @@ def diff(self, other_profile: NumericStatsMixin, options: Dict = None) -> Dict:
                other_profile.variance,
                other_profile.match_count,
            ),
+            "psi": self._calculate_psi(),


call static method in def diff()

taylorfturner · 2022-10-13T15:32:53Z

dataprofiler/profilers/numerical_column_stats.py

+    @staticmethod
+    def _calculate_psi():
+        raise NotImplementedError()
+
    def _update_variance(


this is where PSI actually gets calculated

JGSweets · 2022-11-01T16:48:15Z

dataprofiler/profilers/numerical_column_stats.py

+        self_count = sum1 / mean1
+        other_count = sum2 / mean2


If the intent is to get the count, we could just sum the bin_counts instead. otherwise we might lose floating point accuracies, no? Also:

def mean(self) -> float: """Return mean value.""" if self.match_count == 0: return 0 return float(self.sum) / self.match_count

so could just use match_count as well.

Perf! I'll use match_count -- less code and reuse what already exists

dataprofiler/profilers/numerical_column_stats.py

JGSweets · 2022-11-01T21:43:09Z

dataprofiler/profilers/numerical_column_stats.py

+
+        if regenerate_histogram:
+            len_self_bin_counts = 0
+            if len(self._stored_histogram["histogram"]["bin_counts"]) > 0:


Is this if statement needed?

JGSweets · 2022-11-01T21:44:35Z

dataprofiler/profilers/numerical_column_stats.py

+                new_self_histogram["bin_counts"] = histogram["bin_counts"]
+                new_self_histogram["bin_edges"] = histogram["bin_edges"]
+
+            if len_self_bin_counts == num_psi_bins:


Instead of this if, could set it above the first if, and it will get reassigned given the criteria

totally fair -- updating now

JGSweets · 2022-11-01T21:45:18Z

dataprofiler/profilers/numerical_column_stats.py

+                new_other_histogram["bin_edges"] = histogram["bin_edges"]
+                new_other_histogram["bin_counts"] = histogram["bin_counts"]
+
+            if not histogram_edges_not_equal:


totally fair -- updating now

This can reduce 3 ifs down to one

JGSweets · 2022-11-01T21:48:58Z

dataprofiler/profilers/numerical_column_stats.py

@@ -369,6 +369,93 @@ def diff(self, other_profile: NumericStatsMixin, options: Dict = None) -> Dict:
                "and '{}'".format(cls.__name__, other_profile.__class__.__name__)
            )

+        new_self_histogram = {"bin_counts": None, "bin_edges": None}


I think this is fine, but one thing I would consider is the rest of the api functions in diff are purely padding the data with all the checks within.

To keep things similar, unless the histograms were used elsewhere, I'd consider putting all of this in calculate_psi

nah that's good -- lemme see what I can do right quick and push up

dataprofiler/profilers/numerical_column_stats.py

JGSweets · 2022-11-01T21:52:23Z

dataprofiler/profilers/numerical_column_stats.py

+            )
+            invalid_stats = True
+
+        if invalid_stats:


Instead of an extra if could we put it inside the previous?

lol true 🤦

fixed in next push

placeholder

f6be48b

taylorfturner added Work In Progress Solution is being developed New Feature A feature addition not currently in the library labels Oct 13, 2022

taylorfturner self-assigned this Oct 13, 2022

taylorfturner requested review from JGSweets, ksneab7, micdavis and tyfarnan as code owners October 13, 2022 15:01

taylorfturner changed the title ~~WIP Adding PSI to diff report~~ [WIP] Adding PSI to diff report Oct 13, 2022

adding placeholders

336a8a2

taylorfturner commented Oct 13, 2022

View reviewed changes

taylorfturner added 6 commits October 13, 2022 14:03

DRY-er code

7584838

logic placeholder

3394a0f

reformat

4b4f165

end of day catch up

0358ecc

catch up

eae37b5

Merge branch 'main' into feature/psi

2a19fc1

taylorfturner changed the title ~~[WIP] Adding PSI to diff report~~ Adding PSI to diff report Oct 19, 2022

taylorfturner added 10 commits October 19, 2022 13:22

Merge branch 'main' into feature/psi

3ce26cf

catch up

c311ebc

conditionally regen

b92793b

conditionally regen

4a3f34d

black reformatting

f418d5f

black reformatting

66c4122

commit

6a534aa

commit

943def3

commit

fd2e425

commit

b439472

JGSweets reviewed Nov 1, 2022

View reviewed changes

dataprofiler/profilers/numerical_column_stats.py Show resolved Hide resolved

taylorfturner added the Work In Progress Solution is being developed label Nov 1, 2022

taylorfturner added 8 commits November 1, 2022 14:22

format

040ce6d

docstring and .match_count

1a2db04

psi clean up bin edges and max/min

e1d367e

psi values based on manual calc

251dff7

psi values based on manual calc

53e7355

clean up logic on regen_hist

54ca76f

clean up logic on regen_hist

2730d0b

clean up variable

a7b0d46

taylorfturner requested review from tyfarnan and JGSweets November 1, 2022 21:35

JGSweets reviewed Nov 1, 2022

View reviewed changes

dataprofiler/profilers/numerical_column_stats.py Show resolved Hide resolved

JGSweets reviewed Nov 1, 2022

View reviewed changes

taylorfturner added 3 commits November 1, 2022 18:03

clean up ifs

33b1710

classmethod

19cdd35

classmethod

b3672aa

taylorfturner requested a review from JGSweets November 2, 2022 00:26

fix debugging remnant

e2fba6d

taylorfturner removed the Work In Progress Solution is being developed label Nov 2, 2022

JGSweets approved these changes Nov 2, 2022

View reviewed changes

JGSweets enabled auto-merge (squash) November 2, 2022 03:13

ksneab7 approved these changes Nov 2, 2022

View reviewed changes

JGSweets merged commit 6a407af into capitalone:main Nov 2, 2022

taylorfturner mentioned this pull request Nov 2, 2022

Updated setup.cfg mypy flags and resolved related errors. #703

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding `PSI` to `diff` report #688

Adding `PSI` to `diff` report #688

taylorfturner commented Oct 13, 2022 •

edited

Loading

taylorfturner Oct 13, 2022

JGSweets Oct 13, 2022

taylorfturner Oct 13, 2022

JGSweets Oct 13, 2022

taylorfturner Oct 13, 2022

taylorfturner Oct 19, 2022 •

edited

Loading

taylorfturner Oct 13, 2022

taylorfturner Oct 13, 2022

JGSweets Nov 1, 2022 •

edited

Loading

taylorfturner Nov 1, 2022

JGSweets Nov 1, 2022

JGSweets Nov 1, 2022

taylorfturner Nov 1, 2022

JGSweets Nov 1, 2022

taylorfturner Nov 1, 2022

JGSweets Nov 1, 2022

taylorfturner Nov 2, 2022

JGSweets Nov 1, 2022

taylorfturner Nov 1, 2022

JGSweets Nov 1, 2022

taylorfturner Nov 1, 2022

taylorfturner Nov 1, 2022

Adding PSI to diff report #688

Adding PSI to diff report #688

Conversation

taylorfturner commented Oct 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taylorfturner Oct 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JGSweets Nov 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Adding `PSI` to `diff` report #688

Adding `PSI` to `diff` report #688

taylorfturner commented Oct 13, 2022 •

edited

Loading

taylorfturner Oct 19, 2022 •

edited

Loading

JGSweets Nov 1, 2022 •

edited

Loading