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

REF/CLN: collect imports at top of file, de-duplicate imports #5805

Merged
merged 2 commits into from May 29, 2019

Conversation

jbrockmendel
Copy link
Contributor

No description provided.

@pep8speaks
Copy link

pep8speaks commented May 29, 2019

Hello @jbrockmendel! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-05-29 04:42:47 UTC

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.004%) to 84.986% when pulling b65c971 on jbrockmendel:E4 into 321619e on statsmodels:master.

@codecov
Copy link

codecov bot commented May 29, 2019

Codecov Report

Merging #5805 into master will increase coverage by <.01%.
The diff coverage is 54.05%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5805      +/-   ##
==========================================
+ Coverage   82.46%   82.47%   +<.01%     
==========================================
  Files         595      595              
  Lines       93776    93765      -11     
  Branches    10353    10353              
==========================================
- Hits        77333    77328       -5     
+ Misses      14046    14040       -6     
  Partials     2397     2397
Impacted Files Coverage Δ
statsmodels/sandbox/distributions/transformed.py 0% <ø> (ø) ⬆️
statsmodels/sandbox/archive/tsa.py 0% <ø> (ø) ⬆️
statsmodels/tsa/coint_tables.py 82.05% <ø> (ø) ⬆️
statsmodels/sandbox/archive/linalg_covmat.py 0% <0%> (ø) ⬆️
.../sandbox/distributions/examples/ex_mvelliptical.py 0% <0%> (ø) ⬆️
...atsmodels/sandbox/regression/ols_anova_original.py 0% <0%> (ø) ⬆️
statsmodels/sandbox/tsa/example_arma.py 0% <0%> (ø) ⬆️
statsmodels/sandbox/tsa/try_var_convolve.py 0% <0%> (ø) ⬆️
...tsmodels/sandbox/nonparametric/densityorthopoly.py 0% <0%> (ø) ⬆️
statsmodels/sandbox/distributions/genpareto.py 0% <0%> (ø) ⬆️
... and 14 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 321619e...b65c971. Read the comment docs.

2 similar comments
@codecov
Copy link

codecov bot commented May 29, 2019

Codecov Report

Merging #5805 into master will increase coverage by <.01%.
The diff coverage is 54.05%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5805      +/-   ##
==========================================
+ Coverage   82.46%   82.47%   +<.01%     
==========================================
  Files         595      595              
  Lines       93776    93765      -11     
  Branches    10353    10353              
==========================================
- Hits        77333    77328       -5     
+ Misses      14046    14040       -6     
  Partials     2397     2397
Impacted Files Coverage Δ
statsmodels/sandbox/distributions/transformed.py 0% <ø> (ø) ⬆️
statsmodels/sandbox/archive/tsa.py 0% <ø> (ø) ⬆️
statsmodels/tsa/coint_tables.py 82.05% <ø> (ø) ⬆️
statsmodels/sandbox/archive/linalg_covmat.py 0% <0%> (ø) ⬆️
.../sandbox/distributions/examples/ex_mvelliptical.py 0% <0%> (ø) ⬆️
...atsmodels/sandbox/regression/ols_anova_original.py 0% <0%> (ø) ⬆️
statsmodels/sandbox/tsa/example_arma.py 0% <0%> (ø) ⬆️
statsmodels/sandbox/tsa/try_var_convolve.py 0% <0%> (ø) ⬆️
...tsmodels/sandbox/nonparametric/densityorthopoly.py 0% <0%> (ø) ⬆️
statsmodels/sandbox/distributions/genpareto.py 0% <0%> (ø) ⬆️
... and 14 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 321619e...b65c971. Read the comment docs.

@codecov
Copy link

codecov bot commented May 29, 2019

Codecov Report

Merging #5805 into master will increase coverage by <.01%.
The diff coverage is 54.05%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5805      +/-   ##
==========================================
+ Coverage   82.46%   82.47%   +<.01%     
==========================================
  Files         595      595              
  Lines       93776    93765      -11     
  Branches    10353    10353              
==========================================
- Hits        77333    77328       -5     
+ Misses      14046    14040       -6     
  Partials     2397     2397
Impacted Files Coverage Δ
statsmodels/sandbox/distributions/transformed.py 0% <ø> (ø) ⬆️
statsmodels/sandbox/archive/tsa.py 0% <ø> (ø) ⬆️
statsmodels/tsa/coint_tables.py 82.05% <ø> (ø) ⬆️
statsmodels/sandbox/archive/linalg_covmat.py 0% <0%> (ø) ⬆️
.../sandbox/distributions/examples/ex_mvelliptical.py 0% <0%> (ø) ⬆️
...atsmodels/sandbox/regression/ols_anova_original.py 0% <0%> (ø) ⬆️
statsmodels/sandbox/tsa/example_arma.py 0% <0%> (ø) ⬆️
statsmodels/sandbox/tsa/try_var_convolve.py 0% <0%> (ø) ⬆️
...tsmodels/sandbox/nonparametric/densityorthopoly.py 0% <0%> (ø) ⬆️
statsmodels/sandbox/distributions/genpareto.py 0% <0%> (ø) ⬆️
... and 14 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 321619e...b65c971. Read the comment docs.

@bashtage
Copy link
Member

Cleaning of very old, unloved code.

Ideally, the working examples in sm\sm\examples should be moved to notebooks in sm\examples\notebooks.

@bashtage
Copy link
Member

4 years since the last non-bugfix change.

@bashtage bashtage merged commit 92885fd into statsmodels:master May 29, 2019
#should be replace
# FIXME: importing these patches scipy distribution classes in-place.
# Don't do this.
import statsmodels.sandbox.distributions.sppatch # noqa:F401
Copy link
Member

Choose a reason for hiding this comment

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

don't change this import location
Given that this is monkey patching scipy, the location of the import is important and might change other results.

@josef-pkt
Copy link
Member

those PRs need review

It's not a very productive use of time
@jbrockmendel Take one example and convert it into a notebook instead of changing things all over the place !!!

@jbrockmendel
Copy link
Contributor Author

Take one example and convert it into a notebook

Awaiting comment on #5795.

@jbrockmendel jbrockmendel deleted the E4 branch May 29, 2019 13:53
@josef-pkt
Copy link
Member

#5795 add it as or to a notebook for the docs and we don't have to worry about changes to the original script.
The script file then just needs to be marked for deletion if there is no extra content left.

@jbrockmendel
Copy link
Contributor Author

add it as or to a notebook for the docs

Should "as or" be "as is or"? Wouldn't we want to avoid making the currently-wrong version into a notebook?

@josef-pkt
Copy link
Member

I meant add it as a new notebook or add it to an existing notebook if that location is appropriate.

Based on skimming your changes, they look good, but I didn't check the details, nor what is "wrong" with the current version.
You put your version with cleanup and changes and possibly extra explanations in the notebook, but as in that PR, the existing example script files can be taken as basis for noteboooks.

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

5 participants