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

ENH: Add penalization summary, closes #5461 #5490

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

stevenbw
Copy link

@stevenbw stevenbw commented Feb 3, 2019

Added the add_penal_table method to the summary class which collects the penalization information and returns it as a SimpleTable instance.

@coveralls
Copy link

coveralls commented Feb 3, 2019

Coverage Status

Coverage decreased (-0.01%) to 84.485% when pulling cdb69e7 on stevenbw:penal-summary into 91d71c6 on statsmodels:master.

@codecov
Copy link

codecov bot commented Feb 3, 2019

Codecov Report

Merging #5490 into master will decrease coverage by 0.06%.
The diff coverage is 44.44%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5490      +/-   ##
==========================================
- Coverage   81.99%   81.93%   -0.07%     
==========================================
  Files         586      586              
  Lines       92275    92289      +14     
  Branches    10240     9516     -724     
==========================================
- Hits        75660    75613      -47     
- Misses      14273    14328      +55     
- Partials     2342     2348       +6
Impacted Files Coverage Δ
statsmodels/iolib/summary.py 52.76% <44.44%> (-2.19%) ⬇️
statsmodels/iolib/openfile.py 66.66% <0%> (-9.1%) ⬇️
statsmodels/datasets/utils.py 78.39% <0%> (-4.94%) ⬇️
statsmodels/iolib/foreign.py 70.63% <0%> (-1.99%) ⬇️
statsmodels/base/wrapper.py 89.85% <0%> (-1.45%) ⬇️
...ar/tests/JMulTi_results/parse_jmulti_var_output.py 90.16% <0%> (-1.02%) ⬇️
statsmodels/iolib/tests/test_foreign.py 97.63% <0%> (-0.79%) ⬇️
statsmodels/tsa/tsatools.py 89.16% <0%> (-0.62%) ⬇️
statsmodels/discrete/tests/test_discrete.py 98.67% <0%> (-0.59%) ⬇️
... and 11 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 91d71c6...cdb69e7. Read the comment docs.

1 similar comment
@codecov
Copy link

codecov bot commented Feb 3, 2019

Codecov Report

Merging #5490 into master will decrease coverage by 0.06%.
The diff coverage is 44.44%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5490      +/-   ##
==========================================
- Coverage   81.99%   81.93%   -0.07%     
==========================================
  Files         586      586              
  Lines       92275    92289      +14     
  Branches    10240     9516     -724     
==========================================
- Hits        75660    75613      -47     
- Misses      14273    14328      +55     
- Partials     2342     2348       +6
Impacted Files Coverage Δ
statsmodels/iolib/summary.py 52.76% <44.44%> (-2.19%) ⬇️
statsmodels/iolib/openfile.py 66.66% <0%> (-9.1%) ⬇️
statsmodels/datasets/utils.py 78.39% <0%> (-4.94%) ⬇️
statsmodels/iolib/foreign.py 70.63% <0%> (-1.99%) ⬇️
statsmodels/base/wrapper.py 89.85% <0%> (-1.45%) ⬇️
...ar/tests/JMulTi_results/parse_jmulti_var_output.py 90.16% <0%> (-1.02%) ⬇️
statsmodels/iolib/tests/test_foreign.py 97.63% <0%> (-0.79%) ⬇️
statsmodels/tsa/tsatools.py 89.16% <0%> (-0.62%) ⬇️
statsmodels/discrete/tests/test_discrete.py 98.67% <0%> (-0.59%) ⬇️
... and 11 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 91d71c6...cdb69e7. Read the comment docs.

@stevenbw
Copy link
Author

stevenbw commented Feb 4, 2019

@josef-pkt the build failure appears to be unrelated to the changes I have made. Is there anything I can do to resolve this?

@josef-pkt
Copy link
Member

Just ignore that failure, as do I for the moment. We have an open issue to fix it.

@kshedden
Copy link
Contributor

kshedden commented Feb 5, 2019

The issue is #5457

@stevenbw
Copy link
Author

stevenbw commented Feb 5, 2019

Thanks @josef-pkt @kshedden. Do let me know if there are any changes I should make to the PR.

@josef-pkt
Copy link
Member

@stevenbw The main thing are unit tests and examples to see for which models this works.

I only thought about it for a bit. I think we need additional keyword arguments to override that the method of the results instance is used, e.g. I'm not sure how consistent penalization is at the moment across models, or which additional info will be available.
For example in GLMGam, it might be better to use edf for each column in the summary, alpha is term and not column specific.

edf is effective degrees of freedom which is currently only available in GAM.

@stevenbw
Copy link
Author

Thanks @josef-pkt, I'll add some unit tests and examples.

Do you think it would be better to extend the method for other models with more appropriate summaries in another PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants