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

MAINT: simplify discrete calls to get_robustcov_results #5234

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
36 changes: 17 additions & 19 deletions statsmodels/discrete/discrete_model.py
Expand Up @@ -1102,12 +1102,7 @@ def fit(self, start_params=None, method='newton', maxiter=35,
callback=callback,
**kwargs)

if 'cov_type' in kwargs:
cov_kwds = kwargs.get('cov_kwds', {})
kwds = {'cov_type':kwargs['cov_type'], 'cov_kwds':cov_kwds}
else:
kwds = {}
discretefit = PoissonResults(self, cntfit, **kwds)
discretefit = PoissonResults(self, cntfit)
return PoissonResultsWrapper(discretefit)

@Appender(DiscreteModel.fit_regularized.__doc__)
Expand Down Expand Up @@ -1520,20 +1515,21 @@ def fit(self, start_params=None, method='bfgs', maxiter=35,
disp=disp,
full_output=full_output,
callback=callback,
cov_type=cov_type,
use_t=use_t,
cov_kwds=cov_kwds,
**kwargs)

if use_transparams and method not in ["newton", "ncg"]:
self._transparams = False
mlefit._results.params[-1] = np.exp(mlefit._results.params[-1])
# ensure cov_params are re-evaluated with updated params
delattr(mlefit._results, "cov_type")

gpfit = GeneralizedPoissonResults(self, mlefit._results)
gpfit = GeneralizedPoissonResults(self, mlefit._results,
cov_type=cov_type, use_t=use_t,
cov_kwds=cov_kwds)
result = GeneralizedPoissonResultsWrapper(gpfit)

if cov_kwds is None:
cov_kwds = {}

result._get_robustcov_results(cov_type=cov_type,
use_self=True, use_t=use_t, **cov_kwds)
return result

@Appender(DiscreteModel.fit_regularized.__doc__)
Expand Down Expand Up @@ -3307,19 +3303,19 @@ def fit(self, start_params=None, method='bfgs', maxiter=35,
mlefit = super(NegativeBinomialP, self).fit(start_params=start_params,
maxiter=maxiter, method=method, disp=disp,
full_output=full_output, callback=callback,
cov_type=cov_type, use_t=use_t, cov_kwds=cov_kwds,
**kwargs)

if use_transparams and method not in ["newton", "ncg"]:
self._transparams = False
mlefit._results.params[-1] = np.exp(mlefit._results.params[-1])
# ensure cov_params are re-evaluated with updated params
delattr(mlefit._results, "cov_type")

nbinfit = NegativeBinomialResults(self, mlefit._results)
nbinfit = NegativeBinomialResults(self, mlefit._results,
cov_type=cov_type, use_t=use_t,
cov_kwds=cov_kwds)
result = NegativeBinomialResultsWrapper(nbinfit)

if cov_kwds is None:
cov_kwds = {}
result._get_robustcov_results(cov_type=cov_type,
use_self=True, use_t=use_t, **cov_kwds)
return result

@Appender(DiscreteModel.fit_regularized.__doc__)
Expand Down Expand Up @@ -3464,6 +3460,8 @@ def __init__(self, model, mlefit, cov_type='nonrobust', cov_kwds=None,
if cov_kwds is None:
cov_kwds = {}
from statsmodels.base.covtype import get_robustcov_results
# TODO: use self._get_robustcov_results? Only difference
# with base class is that base class passes use_t
get_robustcov_results(self, cov_type=cov_type, use_self=True,
**cov_kwds)

Expand Down
32 changes: 32 additions & 0 deletions statsmodels/discrete/tests/test_discrete.py
Expand Up @@ -2404,3 +2404,35 @@ def test_t_test():

assert_allclose(t1.effect, t2.effect)
assert_allclose(f1.statistic, f2.statistic)


@pytest.mark.parametrize('method', ['newton', 'bfgs'])
@pytest.mark.parametrize('cov_type', ['nonrobust', 'HC0', 'HC1', 'HC2', 'HC3'])
@pytest.mark.parametrize('use_t', [True, False])
@pytest.mark.parametrize('use_transparams', [True, False])
@pytest.mark.parametrize('mod_cls', [NegativeBinomialP,
GeneralizedPoisson,
Poisson])
def test_cov_types_attached(mod_cls, use_transparams, use_t, cov_type, method):
# GH#5234 Test that cov_type and use_t are correctly attached to the
# results of `model.fit`

# avoid runtime warning in cases where use_transparams is ignored
use_transparams = use_transparams and method not in ['newton', 'ncg']

# Data construction based
# on TestNegativeBinomialPL1Compatability.setup_class
rand_data = sm.datasets.randhie.load(as_pandas=False)

rand_exog = rand_data.exog.view(float).reshape(len(rand_data.exog), -1)
rand_exog_st = (rand_exog - rand_exog.mean(0)) / rand_exog.std(0)
rand_exog = sm.add_constant(rand_exog_st, prepend=True)

# Use only first 500 observations to trim runtime
model = mod_cls(rand_data.endog[:500], rand_exog[:500])
result = model.fit(method=method, cov_type=cov_type, use_t=use_t,
use_transparams=use_transparams,
disp=False)

assert result.use_t is use_t
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All four of the use_t = True, cov_type='nonrobust' cases fail under master. @josef-pkt is that intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@josef-pkt insights here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bashtage any idea if the existing behavior is correct? If so, what am I missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@josef-pkt when you get a chance this is a specifically-your-attention-needing thing. Is the existing behavior intentional?

Copy link
Member

Choose a reason for hiding this comment

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

This might be a bug as in #5331

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This might be a bug as in #5331

Neat. Before I dig into this, is the simplification+unification in this PR something you're open to?

assert result.cov_type == cov_type