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.

Sign up for GitHub

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

Jump to bottom

CLN: Fixes for future SciPY and pandas #6414

Merged
merged 1 commit into from Jan 15, 2020
Merged

CLN: Fixes for future SciPY and pandas #6414

merged 1 commit into from Jan 15, 2020

Conversation

bashtage
Copy link
Member

Silence new warnings produced by pandas 1.0+
Ensure starting values respect builds in HoltWinters

  • code/documentation is well formatted.
  • properly formatted commit message. See
    NumPy's guide.

Notes:

  • It is essential that you add a test when making code changes. Tests are not
    needed for doc changes.
  • When adding a new function, test values should usually be verified in another package (e.g., R/SAS/Stata).
  • When fixing a bug, you must add a test that would produce the bug in master and
    then show that it is fixed with the new code.
  • New code additions must be well formatted. Changes should pass flake8. If on Linux or OSX, you can
    verify you changes are well formatted by running 
    git diff upstream/master -u -- "*.py" | flake8 --diff --isolated
    assuming flake8 is installed. This command is also available on Windows
    using the Windows System for Linux once flake8 is installed in the
    local Linux environment. While passing this test is not required, it is good practice and it help
    improve code quality in statsmodels.
  • Docstring additions must render correctly, including escapes and LaTeX.

@coveralls
Copy link

coveralls commented Jan 15, 2020 •
edited

Coverage Status

Coverage increased (+0.002%) to 87.459% when pulling b35b88d on bashtage:future-fixes into 76da0ea on statsmodels:master.

@josef-pkt
Copy link
Member

don't merge this before a review

AFAICS, adding asarray introduces bugs in the plots that are not unit tested, because we only smoke test plots.

My computer is not usable at the moment, so it will take a day until I can look at it.

josef-pkt
josef-pkt reviewed Jan 15, 2020
View reviewed changes
statsmodels/graphics/regressionplots.py Outdated
@@ -406,15 +406,17 @@ def plot_partregress(endog, exog_i, exog_others, data=None,
# all arrays or pandas-like

if RHS_isemtpy:
endog = np.asarray(endog)
exog_i = np.asarray(exog_i)
ax.plot(endog, exog_i, 'o', **kwargs)
fitted_line = OLS(endog, exog_i).fit()
x_axis_endog_name = 'x' if isinstance(exog_i, np.ndarray) else exog_i.name
y_axis_endog_name = 'y' if isinstance(endog, np.ndarray) else endog.design_info.column_names[0]
Copy link
Member

Choose a reason for hiding this comment

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

with the asarray above we always have np.ndarray and we loose the names which should be used in the plots.

Why do we need the asarray? What changed in pandas?

@codecov
Copy link

codecov bot commented Jan 15, 2020 •
edited

Codecov Report

Merging #6414 into master will increase coverage by 0.01%.
The diff coverage is 90.47%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6414      +/-   ##
==========================================
+ Coverage   85.07%   85.08%   +0.01%     
==========================================
  Files         642      642              
  Lines      103364   103501     +137     
  Branches    11284    11312      +28     
==========================================
+ Hits        87933    88065     +132     
  Misses      12957    12957              
- Partials     2474     2479       +5
Impacted Files Coverage Δ
statsmodels/tsa/tests/test_holtwinters.py 100% <ø> (ø) ⬆️
statsmodels/graphics/regressionplots.py 82.09% <100%> (+0.08%) ⬆️
statsmodels/graphics/tests/test_regressionplots.py 98.31% <100%> (ø) ⬆️
statsmodels/base/data.py 91.33% <100%> (ø) ⬆️
statsmodels/tsa/holtwinters.py 90.9% <100%> (+0.17%) ⬆️
statsmodels/sandbox/predict_functional.py 77.77% <100%> (ø) ⬆️
statsmodels/base/tests/test_generic_methods.py 97.78% <33.33%> (ø) ⬆️
statsmodels/regression/tests/test_recursive_ls.py 99.57% <0%> (-0.43%) ⬇️
statsmodels/regression/recursive_ls.py 94.69% <0%> (+0.87%) ⬆️

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 76da0ea...8fdf47d. Read the comment docs.

@bashtage
Copy link
Member Author

AFAICS, adding asarray introduces bugs in the plots that are not unit tested, because we only smoke test plots.

pd.Series([1,2,3])[:,None] will not be allowed in the future. Most of the plotting is probably a MPL issue, so I'll defer these.

@bashtage bashtage force-pushed the future-fixes branch 3 times, most recently from b35b88d to 4ecea55 Compare January 15, 2020 09:48
@bashtage
CLN: Fixes for future SciPY and pandas
8fdf47d 
Silence new warnings produced by pandas 1.0+
Ensure starting values respect builds in HoltWinters
@bashtage bashtage merged commit 6e97535 into statsmodels:master Jan 15, 2020
@bashtage bashtage deleted the future-fixes branch January 15, 2020 09:49
@bashtage bashtage added this to the 0.11 milestone Jan 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@josef-pkt josef-pkt josef-pkt left review comments

Assignees
No one assigned
Labels
maintenance
Projects
None yet
Milestone
0.11
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@bashtage @coveralls @josef-pkt