-
Notifications
You must be signed in to change notification settings - Fork 1
[ZC-617]: add audience ratio measure #412
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
Conversation
2ab1a49 to
b11371c
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #412 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 20 19 -1
Lines 4920 4933 +13
=========================================
+ Hits 4920 4933 +13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
6b34888 to
ceedaef
Compare
| jobs: | ||
| build: | ||
| runs-on: ubuntu-20.04 | ||
| runs-on: ubuntu-22.04 |
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.
gh removed runner on 20.04
| [testenv:lint] | ||
| skip_install = True | ||
| basepython = python3.8 | ||
| basepython = python3.10 |
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.
default python version in ubuntu 22.04 is 3.10
gergness
left a comment
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.
Looks good, a minor suggestion
| # --- form consistent with `.row_display_order()` and we'll elaborate this when | ||
| # --- we add sort-by-value to columns. | ||
| collation_method = dimensions[1].order_spec.collation_method | ||
| # fmt: off |
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.
!dm black 😢
For my curiosity, there's a native switch statement in newer versions of python right? How far off are we from being able to use it?
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.
yes there's the match statement that is introduced in 3.10, we could use that but we need to drop 3.8 and 3.9 support, that is feasible to me considering that exporter works on 3.11 now, but dunno if cube is used even directly in other lib like pycrunch or scrunch, so I'd live it with this horrible thing for now :D ... and yes !dm black
| This measure (also known as "Index") is a 2D np.float64 ndarray of the ratio of the | ||
| proportions of the first column to the proportions of the second column. | ||
|
|
||
| Audience ratio (Index) is a convenient way of showing the ratio of the Target % and |
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.
This along with smoothing and the reference column statistical significance really makes me wish we had time to design a way to pass arguments into measures better. (so the audience measure would specify which column was the control column instead of being fixed to the second one)
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.
yes I do agree, and I had a PR ready for pw signif 2 years ago, but it has been frozen since then because the FE needed to change to much stuff, and since then no PRD has been approved to take care of this issue :(
No description provided.