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

Conversation

jbrockmendel
Copy link
Contributor

get_robustcov_results is called in LikelihoodModelResults.__init__. Avoid re-calling it to make things less-stateful.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.002%) to 84.2% when pulling f32994b on jbrockmendel:discrete_calls into a764908 on statsmodels:master.

@coveralls
Copy link

coveralls commented Sep 20, 2018

Coverage Status

Coverage increased (+0.001%) to 88.395% when pulling 8dda7c7 on jbrockmendel:discrete_calls into 52b142c on statsmodels:master.

@codecov
Copy link

codecov bot commented Sep 20, 2018

Codecov Report

Merging #5234 into master will decrease coverage by 1.98%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5234      +/-   ##
==========================================
- Coverage   86.07%   84.09%   -1.99%     
==========================================
  Files         698      613      -85     
  Lines      118309    97619   -20690     
  Branches    13131    10657    -2474     
==========================================
- Hits       101840    82093   -19747     
+ Misses      13707    13118     -589     
+ Partials     2762     2408     -354     
Impacted Files Coverage Δ
statsmodels/discrete/discrete_model.py 90.64% <100.00%> (+2.34%) ⬆️
statsmodels/discrete/tests/test_discrete.py 99.85% <100.00%> (+<0.01%) ⬆️
statsmodels/stats/descriptivestats.py 23.68% <0.00%> (-63.45%) ⬇️
statsmodels/stats/base.py 52.94% <0.00%> (-14.41%) ⬇️
statsmodels/nonparametric/tests/test_kernels.py 89.13% <0.00%> (-10.87%) ⬇️
statsmodels/tools/docstring.py 71.12% <0.00%> (-8.30%) ⬇️
statsmodels/graphics/gofplots.py 83.95% <0.00%> (-7.64%) ⬇️
statsmodels/stats/tests/test_diagnostic.py 92.65% <0.00%> (-7.19%) ⬇️
statsmodels/stats/_lilliefors.py 91.13% <0.00%> (-6.23%) ⬇️
statsmodels/graphics/correlation.py 84.84% <0.00%> (-6.07%) ⬇️
... and 376 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 52b142c...8dda7c7. Read the comment docs.

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?

@josef-pkt
Copy link
Member

The general idea looks good.
What I don't know is whether we want to have the cov_type keywords twice, once in the super call and once when creating the model specific results instance.
My "impression" is that it might be enough to use it once or that it should be used only once, but I didn't go through those details to see how this works.
I don't remember whether there was something tricky in the computation, but this needs to be careful because the extra dispersion parameter in GP and NB is transformed during optimization but we use the untransformed parameterization for the results. This means that the super(..).fit call is not using the "results parameterization" and cannot compute the appropriate cov_params.

There are some tricky parts to the internal temporary transformation, but I don't remember at the moment what the status is. e.g. #3747

@jbrockmendel
Copy link
Contributor Author

@bashtage not-just-cln PRs this (#5234), #4936, #5205, #5241, #5785, #5678, #5264. With the exception of the first and last, these are all about code de-duplication.

@josef-pkt
Copy link
Member

I'm pretty sure this is either incorrect or fragile because of the extra dispersion parameter

@josef-pkt
Copy link
Member

and I guess it computes the sandwich cov_params twice, the first time incorrectly

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

3 participants