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

Statsmodels interface #39

Merged
merged 61 commits into from
Aug 19, 2016
Merged

Statsmodels interface #39

merged 61 commits into from
Aug 19, 2016

Conversation

mortonjt
Copy link
Collaborator

This is picking up from #22

This basically sums up all of the PRs that have been reviewed so far.

The last thing that needs to happen here is the IPython notebooks need to be rebuilt with the existing code. I'll let you guys know when that is ready.

@mortonjt mortonjt mentioned this pull request Aug 18, 2016
3 tasks
@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 99.606% when pulling 47f577f on regression_results into f13954d on master.

@josenavas
Copy link
Member

Does this need to be review/merged?

@mortonjt
Copy link
Collaborator Author

A review would be great!

feature_names=table.columns)


def mixedlm(formula, table, metadata, tree, groups, **kwargs):
Copy link
Member

Choose a reason for hiding this comment

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

just curious, isn't this method basically the same as the ols() method short of the specific method executed under the smf namespace?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes and no.

The idea was to provide the same sort of interface to the user, so that they can easily swap methods in and out.
The only difference between mixedlm and ols from an API point of view is that mixedlm has an required argument groups.

Copy link
Member

Choose a reason for hiding this comment

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

ah, okay

@mortonjt
Copy link
Collaborator Author

Don't merge just yet

I just realized that I need to throw a ValueError in case there are zeros in the data.

for r in self.results:
p = r.pvalues
p.name = r.model.endog_names
self.pvalues = self.pvalues.append(p)
Copy link
Member

Choose a reason for hiding this comment

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

does append support inplace?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ElDeveloper
Copy link
Member

👍

Adding more documentation explaining ols and mixedlm
Addressing @wasade's comments
@mortonjt
Copy link
Collaborator Author

Ok. All comments have been addressed.

Note that I've forced the scikit-bio dependency to be 0.4.2, since there is an issue with shear/prune in 0.5.0. I'll raise a detailed bug report once this PR is merged.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 99.611% when pulling adc911f on regression_results into f13954d on master.

@mortonjt
Copy link
Collaborator Author

The changelog has been updated. This is ready for final review/merge.

Note that this is the last major change before the alpha release.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 99.611% when pulling 38ae180 on regression_results into f13954d on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 99.611% when pulling c9e582b on regression_results into f13954d on master.

@@ -1,9 +1,15 @@
# gneiss changelog
## Version 0.1.0 (changes since 0.1.0 go here)
Copy link
Member

Choose a reason for hiding this comment

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

If you are tagging the repo as of the merge of this PR, then it may make more sense to remove these two lines, and (below) update the "Version 0.0.2 (changes since 0.0.2 go here)" to "Version 0.0.2".

@ElDeveloper
Copy link
Member

Looks good to me overall 👍

@mortonjt
Copy link
Collaborator Author

Sweet!

@coveralls
Copy link

coveralls commented Aug 19, 2016

Coverage Status

Coverage increased (+0.3%) to 99.611% when pulling af3627f on regression_results into f13954d on master.

@mortonjt
Copy link
Collaborator Author

Can we merge this?

@wasade wasade merged commit 69e8802 into master Aug 19, 2016
@mortonjt mortonjt mentioned this pull request Aug 23, 2016
@mortonjt mortonjt deleted the regression_results branch March 4, 2017 19:01
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

6 participants