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

discreteResults margeff method not tests, obsolete #781

Closed
josef-pkt opened this issue Apr 23, 2013 · 3 comments
Closed

discreteResults margeff method not tests, obsolete #781

josef-pkt opened this issue Apr 23, 2013 · 3 comments
Milestone

Comments

@josef-pkt
Copy link
Member

the margeff method has zero test coverage, get_margeff is the new method

is margeff obsolete ?
or does it need unit tests comparing to separated out Margins class ?

@josef-pkt
Copy link
Member Author

@jseabold Why do we deprecate a broken function ?

for example Logit case here (there is PR #892 for fixing the examples)
http://statsmodels.sourceforge.net/devel/examples/notebooks/generated/example_discrete.html

logit_mod = sm.Logit(spector_data.endog, spector_data.exog)
logit_res = logit_mod.fit()
print 'Parameters: ', logit_res.params
print 'Marginal effects: ', logit_res.margeff()

---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-5-761e1b345c46> in <module>()
      2 logit_res = logit_mod.fit()
      3 print 'Parameters: ', logit_res.params
----> 4 print 'Marginal effects: ', logit_res.margeff()

e:\josef\eclipsegworkspace\statsmodels-git\statsmodels-all-new2\statsmodels\statsmodels\discrete\discrete_model.pyc in margeff(self, at, method, atexog, dummy, count)
   2111                                                 self.model._derivative_exog,
   2112                                                 dummy_ind, count_ind,
-> 2113                                                 method)
   2114         # don't care about at constant
   2115         self.margeff_cov = margeff_cov[ind][:, ind]

TypeError: margeff_cov_with_se() takes exactly 10 arguments (9 given)

Optimization terminated successfully.
         Current function value: 0.402801
         Iterations 7
Parameters:  [  2.82611259   0.09515766   2.37868766 -13.02134686]
Marginal effects: 

e:\josef\eclipsegworkspace\statsmodels-git\statsmodels-all-new2\statsmodels\statsmodels\discrete\discrete_model.py:2043: FutureWarning: This method is deprecated and will be removed in 0.6.0. Use get_margeff instead
  " Use get_margeff instead", FutureWarning)

@jseabold
Copy link
Member

I don't think that case worked before either, and I didn't feel like fixing it since it's deprecated. I deprecated it because I had demo'd it in a few talks. I'm fine with removing it though. Or just raising a NotImplementedError to use get_margeff or something.

@josef-pkt
Copy link
Member Author

I think removing it would be best in that case. I think it's strange to get a deprecation warning for something that doesn't and didn't work. If it didn't work before, then there should not be any code depending on it.

One possibility would be to delegate to the new class, if we want to keep the deprecation warning.

josef-pkt added a commit that referenced this issue Jun 23, 2013
MAINT: Remove broken function. Keep deprecation. Closes #781. (PR #894)
PierreBdR pushed a commit to PierreBdR/statsmodels that referenced this issue Sep 2, 2014
PierreBdR pushed a commit to PierreBdR/statsmodels that referenced this issue Sep 2, 2014
MAINT: Remove broken function. Keep deprecation. Closes statsmodels#781. (PR statsmodels#894)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants