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

De-duplicate a bunch of identical code #4458

Merged
merged 1 commit into from
Jun 12, 2018

Conversation

jbrockmendel
Copy link
Contributor

The first half of irf_errband_mc is copy/paste identical to irf_resim, so let's go ahead and use that instead.

Also make a helper method for a commonly-used expression in irf.

This does not:

  • clean up any formatting
  • fix the AttributeError that irf_errband_mc or irf_resim would raise if it were ever called.
  • fix the TypeError that irf_errband_mc would raise if it that AttributeError were fixed.

@coveralls
Copy link

coveralls commented Apr 10, 2018

Coverage Status

Coverage increased (+0.03%) to 82.723% when pulling 3c2b110 on jbrockmendel:resim into 98d67db on statsmodels:master.

@codecov-io
Copy link

Codecov Report

Merging #4458 into master will increase coverage by 0.02%.
The diff coverage is 45.45%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4458      +/-   ##
==========================================
+ Coverage   80.06%   80.09%   +0.02%     
==========================================
  Files         560      560              
  Lines       84369    84347      -22     
  Branches     9559     9554       -5     
==========================================
+ Hits        67551    67554       +3     
+ Misses      14597    14572      -25     
  Partials     2221     2221
Impacted Files Coverage Δ
statsmodels/tsa/vector_ar/var_model.py 85.11% <0%> (+1.58%) ⬆️
statsmodels/tsa/vector_ar/irf.py 54.36% <50%> (+2.4%) ⬆️
statsmodels/stats/descriptivestats.py 24.13% <0%> (ø) ⬆️

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 98d67db...3c2b110. Read the comment docs.

@jbrockmendel
Copy link
Contributor Author

@ChadFulton pls take a look when you get a chance

@ChadFulton
Copy link
Member

LGTM, I think very straightforward, will merge tonight (or tomorrow if I forget) unless I hear otherwise.

@josef-pkt
Copy link
Member

LGTM

@ChadFulton
Copy link
Member

Thanks @jbrockmendel for cleanup and @josef-pkt for review!

@ChadFulton ChadFulton merged commit 3c2ea25 into statsmodels:master Jun 12, 2018
@jbrockmendel jbrockmendel deleted the resim branch June 12, 2018 01:56
@josef-pkt josef-pkt added this to the 0.10 milestone Sep 16, 2018
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.

5 participants