SUMM/REF: Review discrete_model

[josef-pkt]

some issues while looking at discrete models, specifically countmodels for extension in GSOC 2017

• k_extra, extra parameters in margins, BUG/ENH: margins, get_margeff for negative binomial, generalized poisson #3809 -> fixed for NB, GP in BUG/ENH: discrete margins, handle extra param (e.g. NegativeBinomial) #3811, still open two-exog models
• uses endog_names instead of param_names
• fit calls result._get_robustcov_results, can we use the results.__init__ code for this?
• ...
• TST: discrete margins has unit tests against Stata only for Logit, no other model has margeff unit tests
• ...

[jbrockmendel]

Some issues with PRs mergeable or close to it:

• Cleanup and flake8 fixes for discrete_model #3792 Cleanup and flake8 fixes for discrete_model

  • Weibull model has been commented-out in its entirety since at least Oct 2012.
  • Avoid using built-in name all as a kwarg.

• Avoid setting model.df_model in results.__init__ #3735 Avoid setting model.df_model in results.__init__
  JP: merged via BUG/REF Avoid changing model.df_model by results #3949

• Remove un-picklable method in __getstate__ #3678 Fixes __getstate__ error in Discrete models fit_regularized save pickle fails #2685/added discrete model fit_regularized pickle test #2686, removes unnecesssary try/except
  JP: Fix merged via BUG fix pickling for discrete fit_regularized #3939

• Refactor discrete_model to avoid duplicate digamma calls #3592, _score_nbin optimization #3582 Redundant calls to scipy.special.digamma in _score_nbin.

  • NegativeBinomial._score_obs has a TODO note saying to replace it with analytic. I can make a new PR for this, or add a commit to Refactor discrete_model to avoid duplicate digamma calls #3592.
  • Refactor discrete_model to avoid duplicate digamma calls #3592 has a lot of commits which you may want to squash. I kept them separate so that each step of algebra could be viewed independently.

• WIP: Dimensions Mixins #3725 dimensions.L1Estimator can be mixed in to the L1FooResults classes.

  • Most of that PR is for demonstration purposes, but L1Estimator (which needs a different name) should be uncontroversial.

• GSOC2017 Generalized Poisson (GP-P) model #3727 mostly adds new code, but also deletes the __main__ block, which will be a win.

  • If the rest of GSOC2017 Generalized Poisson (GP-P) model #3727 needs more review I'll be happy to make a PR that just does the __main__ bit just so we can merge that and move on.
    JP: GSOC2017 Generalized Poisson (GP-P) model #3727 merged via Generalized Poisson model #3795

Other topics in my notes:

• ENH: DiscreteModel callback default #3706 allow user to specifically pass no-callback.

• There are a bunch of issues regarding wrapping of MultinomialResults methods. Many of them have comments to the effect of "Not sure if this has been resolved." Possibly related: BUG: discrete.discrete_model MNLogitResults.summary2() raises ValueError in new smoketest #3651, mnlogit t_test broken, patsy? #669, BUG: f_test and t_test shape mismatch in MNLogit #457, mnlogit t_test broken, patsy? #669, BUG: fix summary2 for MNLogit #3661

• CountModel._derivative_predict does not have test coverage for cases where a) 'ex' in transform, b) 'ey' in transform, c) count_idx is not None. Same for 'ey' in transform for _derivative_predict.

• In CountModel.__init__, instead of delattr(self, 'offset') and delattr(self, 'exposure'), we should set those to zero or None or ... per discussion in Attribute Existence Ambiguity #3787

• Many of the fit and fit_regularized methods could be rendered redundant if we attached attribute _results_class and _results_wrapper_class to the model classes. MultinomialModel and NegativeBinomial would still need their own fit and fit_regularized methods, but everything else could defer to DiscreteModel.

• cov_params_func_l1 might make a good candidate for moving cov_params logic into base.cov_types.

• Request for documentation: L2226 start_params = np.append(start_params, 0.1)
  JP: enhanced to use estimate for extra params, no code comment

• CountModel._get_init_kwds has a comment describing it as a temporary fixup. No idea how long that has been there.
  JP: still unchanged, changing stored exposure will break backwards compatibility

• Several comments in fit methods # TODO: make a function factory to have multiple callbacks. I'll volunteer to put that together if its still desired.

[jbrockmendel]

@josef-pkt pls ping me when you're back from summer break. I'm looking forward to wrapping some of these up.

[josef-pkt]

(I'm back in Canada, with jetlag and a car that didn't quite survive a wet and humid garage, which delays a bit my going back to review and merging :(
(next priority will be on bug fixes in preparation for release and discrete_model changes coming from the new GSOC count models. I only started a partial triage in https://github.com/statsmodels/statsmodels/projects/8 )

[jbrockmendel]

next priority will be on bug fixes in preparation for release and discrete_model changes coming from the new GSOC count models

Gotcha. Does that mean I should leave this be for N weeks? Or is slow-but-nonzero momentum an option?

[josef-pkt]

several of the bugs listed here have been fixed, refactorings mostly open (mostly for 0.10)

[josef-pkt]

two more details

• MultinomialModel.fit does not use super(...).fit, instead calls base.LikelihoodModel.fit directly
• DiscreteModel._check_perfect_pred computes fittedvalues even if not needed.

[josef-pkt]

I added a few inline comments by editing list in @jbrockmendel's comment

[josef-pkt]

bump, to check what's left and we might still want to change.

My recent PRs are enhancements to discrete, but not much refactoring and code streamlining yet.