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

add cmh and breslow-day tests #9

Merged
merged 7 commits into from Dec 26, 2018
Merged

add cmh and breslow-day tests #9

merged 7 commits into from Dec 26, 2018

Conversation

annepym
Copy link

@annepym annepym commented Sep 26, 2018

Added a separate file including functions for Cochran-Mantel-Haenszel and Breslow-Day tests, per Kelly's request. @nickdeveau94 this is mostly your code.

auditai/cmh.py Outdated Show resolved Hide resolved
@VishalMurali
Copy link
Contributor

@annepym Why are these tests in a different file? We have a file called stats.py for the various statistical tests. It would be convenient to have all our statistical tests in one place.

Copy link
Contributor

@dwww2012 dwww2012 left a comment

Choose a reason for hiding this comment

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

i'm wondering generally if an entirely new file is required. regardless, some small things like variables names and documentation should be improved before merging

auditai/cmh.py Outdated Show resolved Hide resolved
auditai/cmh.py Outdated Show resolved Hide resolved
auditai/cmh.py Outdated Show resolved Hide resolved
auditai/cmh.py Outdated

pcmh = 1 - stats.chi2.cdf(cmh, 1)
return cmh, pcmh

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add an example of these tests in one of our example notebooks? The purpose is to clarify how this test would used in a typical machine learning problem.

@deelin
Copy link

deelin commented Oct 18, 2018

Is this going to be merged in soon? It'd be awesome to actually use this in the upcoming client analysis. (I think this is a public repo, so won't say who the AI analysis is for :) )

@ljbaker
Copy link
Contributor

ljbaker commented Oct 18, 2018

@annepym , totally agree with @deelin. Can you commit the suggestions? We can add examples of the CMH test in a separate commit.

@ljbaker
Copy link
Contributor

ljbaker commented Oct 19, 2018

@deelin , @annepym added some intense docstrings and tests, clarified variable names when possible

@ljbaker
Copy link
Contributor

ljbaker commented Oct 29, 2018

@VishalMurali makes a good point that this should be moved to stats.py

auditai/stats.py Outdated Show resolved Hide resolved
@ljbaker
Copy link
Contributor

ljbaker commented Dec 26, 2018

I've added a notebook that walks through the functions with examples (CMH_test_example.ipynb). I'm going to defend that the utility functions for CMH should have a standalone folder within utils/cmh.py, as the utility functions could be confused with other functions (e.g. utils.crosstabs.crosstab_odds_ratio for calculating odds ratios in most other cases).

Copy link
Contributor

@dwww2012 dwww2012 left a comment

Choose a reason for hiding this comment

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

ok this looks good to me

@ljbaker ljbaker merged commit 433985a into master Dec 26, 2018
@deelin
Copy link

deelin commented Dec 26, 2018

WOOHOO!

@MarkAWard MarkAWard deleted the cmh branch December 27, 2018 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants