Allow users to run one tailed experiments #137

Gabrielcidral1 · 2023-12-20T16:53:11Z

This PR aims to allow users to run 1 tailed power analyses. Currently, there's only the possibility of running 2 tailed.

This is the rationale for the adaptation:

If the actual difference (effect) went in the predicted direction:
The one-tail p-value is half the two-tail p-value. So if the two-tailed p-value is 0.1, the one-tailed p-value is 0.05.
The two-tail p-value is twice the one-tail p-value.
If the actual difference (effect) went opposite to the predicted direction:
The one-tail p-value equals one minus half the two-tailed value. So if the two-tailed p-value is 0.1, the one-tailed p-value is 0.95
The two-tail p-value is twice the one-tail p-value.

From my perspective, the best way is that in case of one-tailed experiments, the user inputs the desired direction (left or right, instead of just 'one-tailed'). In case we want accept 'one-sided', we could guess the side by the sign of the average_effect input. However, there would be a challenge as with current design the analysis class doesn't have the effect information.

Gabrielcidral1 · 2023-12-20T17:31:10Z

Why I can't see the tests? Do I need to deploy the branch?

cluster_experiments/experiment_analysis.py

david26694 · 2023-12-21T08:29:19Z

tests/analysis/test_ols_analysis.py

@@ -14,5 +14,5 @@ def test_binary_treatment():

 def test_get_pvalue():
    analysis_df_full = pd.concat([analysis_df for _ in range(100)])
-    analyser = OLSAnalysis()
+    analyser = OLSAnalysis(hypothesis="left_tailed")


I'd add a separate test, and I'd test the functionality itself of the p-value transformer

as in, check that it is dividing by 2 or doing the other stuff when necessary

david26694 · 2023-12-21T08:30:07Z

.pre-commit-config.yaml

@@ -12,7 +12,7 @@ repos:
    rev: 22.12.0
    hooks:
      - id: black
-        language_version: python3.8
+        language_version: python3.9


is this for your local developement? or is it because of github actions?

david26694 · 2023-12-21T08:31:29Z

Another comment: to me it's a bit hard to remember what does left and right-sided test mean, I'd try to document very well which sign of the effect does it belong to, even creating some notebook in docs that shows the power of the 3 sides for a similar setting

Gabrielcidral1 · 2023-12-21T11:29:51Z

Another comment: to me it's a bit hard to remember what does left and right-sided test mean, I'd try to document very well which sign of the effect does it belong to, even creating some notebook in docs that shows the power of the 3 sides for a similar setting

I found a better way to do this. We can replicate scipy approach to it:

alternative : {'two-sided', 'less', 'greater'}, optional
    Defines the alternative hypothesis.
    The following options are available (default is 'two-sided'):

    * 'two-sided': the means of the distributions underlying the samples
      are unequal.
    * 'less': the mean of the distribution underlying the first sample
      is less than the mean of the distribution underlying the second
      sample.
    * 'greater': the mean of the distribution underlying the first
      sample is greater than the mean of the distribution underlying
      the second sample.

cluster_experiments/experiment_analysis.py

david26694 · 2023-12-22T08:15:54Z

cluster_experiments/experiment_analysis.py

+        elif self.hypothesis == "greater":
+            p_value = p_value_half if treatment_effect >= 0 else 1 - p_value_half
+        elif self.hypothesis == "two-sided":
+            p_value = model_result.pvalues[self.treatment_col]


if we do this, I understand we are not using enum, right? Then I would remove the Enum code

also, an else clause is missing here raising an error

cluster_experiments/experiment_analysis.py

david26694 · 2023-12-22T08:18:02Z

cluster_experiments/experiment_analysis.py

@@ -612,7 +647,9 @@ def analysis_pvalue(self, df: pd.DataFrame, verbose: bool = False) -> float:
        if verbose:
            print(results_mlm.summary())

-        return results_mlm.pvalues[self.treatment_col]
+        p_value = self.pvalue_based_on_hypothesis(results_mlm)
+


Suggested change

for consistency

david26694 · 2023-12-22T08:18:33Z

tests/analysis/test_ols_analysis.py

+
+
+@pytest.mark.parametrize("hypothesis", ["one_sided", "two_sided"])
+def test_get_pvalue_hypothesis(hypothesis):


I would add tests for the pvalue_based_on_hypothesis method too, since it has some logic

codecov-commenter · 2023-12-27T22:48:18Z

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (094cf8c) 96.99% compared to head (2a85493) 96.96%.

Files	Patch %	Lines
cluster_experiments/experiment_analysis.py	96.00%	1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #137      +/-   ##
==========================================
- Coverage   96.99%   96.96%   -0.03%     
==========================================
  Files           9        9              
  Lines         864      890      +26     
==========================================
+ Hits          838      863      +25     
- Misses         26       27       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

david26694

Great job! Some things missing:

There's some commented code in the notebook, I'd remove it
You need to add the notebook in mkdocs.yml
You need to increase the library version (I'd change from 0.10.2 to 0.11.0 or something like this)

cluster_experiments/experiment_analysis.py

tests/analysis/test_hypothesis.py

feat: creating p value based on hypothesis

ad1244b

Gabrielcidral1 changed the title ~~Allow users to run one tailed experiments~~ WIP - Allow users to run one tailed experiments Dec 20, 2023

Gabrielcidral1 marked this pull request as ready for review December 20, 2023 17:27

Gabrielcidral1 marked this pull request as draft December 20, 2023 17:31

Gabrielcidral1 requested review from david26694 and ludovico-lanni December 20, 2023 17:32

david26694 requested changes Dec 21, 2023

View reviewed changes

feat: adapt test

28a1db6

feat: enum, change standard names, apply to all models

a064435

Gabrielcidral1 requested a review from david26694 December 21, 2023 13:24

david26694 requested changes Dec 22, 2023

View reviewed changes

Gabrielcidral1 changed the title ~~WIP - Allow users to run one tailed experiments~~ Allow users to run one tailed experiments Dec 27, 2023

Gabrielcidral1 requested a review from david26694 December 27, 2023 22:39

feat: docs and minor changes

92eacb8

Gabrielcidral1 force-pushed the tailed-experiments branch from 15cb1b1 to 92eacb8 Compare December 27, 2023 22:46

Gabrielcidral1 marked this pull request as ready for review December 28, 2023 09:25

feat: unit test test_pvalue_based_on_hypothesis

a2d7359

Gabrielcidral1 requested a review from pablobd December 28, 2023 10:57

feat: docs

bb51bc0

david26694 requested changes Jan 10, 2024

View reviewed changes

cluster_experiments/experiment_analysis.py Show resolved Hide resolved

cluster_experiments/experiment_analysis.py Show resolved Hide resolved

tests/analysis/test_hypothesis.py Show resolved Hide resolved

minor changes, add tests

2a85493

david26694 approved these changes Jan 12, 2024

View reviewed changes

david26694 merged commit 2024738 into main Jan 12, 2024
4 checks passed

david26694 deleted the tailed-experiments branch January 12, 2024 17:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow users to run one tailed experiments #137

Allow users to run one tailed experiments #137

Gabrielcidral1 commented Dec 20, 2023 •

edited

Loading

Gabrielcidral1 commented Dec 20, 2023

david26694 Dec 21, 2023

david26694 Dec 21, 2023

david26694 Dec 21, 2023

david26694 commented Dec 21, 2023 •

edited

Loading

Gabrielcidral1 commented Dec 21, 2023

david26694 Dec 22, 2023

david26694 Dec 22, 2023

david26694 Dec 22, 2023

david26694 Dec 22, 2023

codecov-commenter commented Dec 27, 2023 •

edited

Loading

david26694 left a comment •

edited

Loading



		@pytest.mark.parametrize("hypothesis", ["one_sided", "two_sided"])
		def test_get_pvalue_hypothesis(hypothesis):

Allow users to run one tailed experiments #137

Allow users to run one tailed experiments #137

Conversation

Gabrielcidral1 commented Dec 20, 2023 • edited Loading

Gabrielcidral1 commented Dec 20, 2023

david26694 Dec 21, 2023

Choose a reason for hiding this comment

david26694 Dec 21, 2023

Choose a reason for hiding this comment

david26694 Dec 21, 2023

Choose a reason for hiding this comment

david26694 commented Dec 21, 2023 • edited Loading

Gabrielcidral1 commented Dec 21, 2023

david26694 Dec 22, 2023

Choose a reason for hiding this comment

david26694 Dec 22, 2023

Choose a reason for hiding this comment

david26694 Dec 22, 2023

Choose a reason for hiding this comment

david26694 Dec 22, 2023

Choose a reason for hiding this comment

codecov-commenter commented Dec 27, 2023 • edited Loading

Codecov Report

david26694 left a comment • edited Loading

Choose a reason for hiding this comment

Gabrielcidral1 commented Dec 20, 2023 •

edited

Loading

david26694 commented Dec 21, 2023 •

edited

Loading

codecov-commenter commented Dec 27, 2023 •

edited

Loading

david26694 left a comment •

edited

Loading