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.

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

Update t_test.py #242

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Update t_test.py #242

wants to merge 1 commit into from

Conversation

mohazahran
Copy link
Collaborator

for high d values the call to power_ttest will invoke the c++ boost lib that will throw an exception (below) that will not be caught here.
We can temporeary circumvent this behavior by saturating the d value at 4 (for statistical_power == 0.9 and pvalue == 0.1) and return
a pre-computed required sample size for this value.

for high d values the call to power_ttest will invoke the c++ boost lib that will throw an exception (below) that will not be caught here.
         We can temporeary circumvent this behavior by saturating the d value at 4 (for statistical_power == 0.9 and pvalue == 0.1) and return
         a pre-computed required sample size for this value.
@@ -458,6 +458,16 @@ def compute_required_sample_size(mean1, mean2, var1, var2, statistical_power, pv
if denominator == 0:
return np.inf
d = np.abs(float(mean1) - float(mean2)) / denominator
'''
Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor Questions:

  • Should we be using " instead of ' as in the docstring?
  • There seems to be an extra indent in the details compared to the '''
  • Please start with capital (For)
  • Typos: temporeary

Main questions:

  • How is 2.24 calculated?
  • What's the plan to actually fix the issue and remove this temporary fix? Is there one?
  • power==0.9 and pvalue==0.1 are very specifix. Should we have something more generic e.g., power>=0.9 and pvalue<=0.1?

Thanks for catching this and trying to fix it here. Is there already an open issue on power_ttest?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants