-
Notifications
You must be signed in to change notification settings - Fork 1
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
Weighted proportions for overlaps 177801573 #280
Weighted proportions for overlaps 177801573 #280
Conversation
* Change one overlaps fixture to include weighted counts alongside overlaps, to demonstrate the difference in z scores expectation
* Change how percentages are calculated for overlaps measures * Use weighted counts instead of (unweighted) overlaps selected counts * Prevent regression by changing a fixture and an expectation in a test
13f9f80
to
297a726
Compare
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.
Per comments
@@ -512,25 +512,29 @@ def test_pairwise_significance_mr_x_mr(self): | |||
def test_pairwise_cat_x_mr_gender_x_all_pets_owned(self): |
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.
I would use the suffix _weighted
where it's needed in the test name. This way we understand that we are testing the cube with a weight
@@ -565,6 +566,7 @@ def p_vals(self): | |||
[ | |||
[ | |||
_PairwiseSignificaneBetweenSubvariablesHelper( | |||
self._cube_measures.weighted_cube_counts.weighted_counts, |
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.
what if weighted_counts
is None
? This is an optional cube measure and if it doesn't exists its value is None. Do we have a test that exercises this case?
* Add `weighted` to the test name * Remove weighted counts from one fixture, to demonstrate that the overlaps column proportions calculation still works (defaults to weighted counts)
Per ticket