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: statespace cleanup, alphabetically #5779

Closed
wants to merge 3 commits into from

Conversation

jbrockmendel
Copy link
Contributor

@ChadFulton this is the second of three statespace branches that have been sitting ready to go for a while, then I'll stop creating more work for you.

@coveralls
Copy link

coveralls commented May 23, 2019

Coverage Status

Coverage increased (+0.01%) to 84.839% when pulling 3a99c6f on jbrockmendel:stests6 into 68d5436 on statsmodels:master.

@codecov
Copy link

codecov bot commented May 23, 2019

Codecov Report

Merging #5779 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5779      +/-   ##
==========================================
+ Coverage    82.3%   82.31%   +<.01%     
==========================================
  Files         593      593              
  Lines       93728    93761      +33     
  Branches    10356    10356              
==========================================
+ Hits        77146    77179      +33     
  Misses      14185    14185              
  Partials     2397     2397
Impacted Files Coverage Δ
statsmodels/tsa/statespace/tests/test_kalman.py 95.36% <100%> (ø) ⬆️
...models/tsa/statespace/tests/test_dynamic_factor.py 97.71% <100%> (+0.06%) ⬆️
...tsmodels/tsa/statespace/tests/test_concentrated.py 100% <100%> (ø) ⬆️
statsmodels/tsa/statespace/tests/test_collapsed.py 96.55% <100%> (+0.01%) ⬆️
...els/tsa/statespace/tests/test_impulse_responses.py 100% <100%> (ø) ⬆️
...models/tsa/statespace/tests/test_initialization.py 100% <100%> (ø) ⬆️
...a/statespace/tests/test_exact_diffuse_filtering.py 98.16% <100%> (+0.01%) ⬆️

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 68d5436...3a99c6f. Read the comment docs.

@codecov
Copy link

codecov bot commented May 23, 2019

Codecov Report

Merging #5779 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5779      +/-   ##
==========================================
+ Coverage    82.3%   82.31%   +<.01%     
==========================================
  Files         593      593              
  Lines       93728    93761      +33     
  Branches    10356    10356              
==========================================
+ Hits        77146    77179      +33     
  Misses      14185    14185              
  Partials     2397     2397
Impacted Files Coverage Δ
statsmodels/tsa/statespace/tests/test_kalman.py 95.36% <100%> (ø) ⬆️
...models/tsa/statespace/tests/test_dynamic_factor.py 97.71% <100%> (+0.06%) ⬆️
...tsmodels/tsa/statespace/tests/test_concentrated.py 100% <100%> (ø) ⬆️
statsmodels/tsa/statespace/tests/test_collapsed.py 96.55% <100%> (+0.01%) ⬆️
...els/tsa/statespace/tests/test_impulse_responses.py 100% <100%> (ø) ⬆️
...models/tsa/statespace/tests/test_initialization.py 100% <100%> (ø) ⬆️
...a/statespace/tests/test_exact_diffuse_filtering.py 98.16% <100%> (+0.01%) ⬆️

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 68d5436...ca52a73. Read the comment docs.

@jbrockmendel
Copy link
Contributor Author

@bashtage additional failure mode ssl.SSLError for get_rdataset.

@bashtage
Copy link
Member

TBH I don't really see the point of switching existing assert_ to assert or assert_equal to assert A == B. It creates a lot of churn and there is effectively 0 gain since NumPy will never drop these functions.

I'm more positive about pytest.raises since this produces more natural function calls.

@jbrockmendel
Copy link
Contributor Author

I didn't count, but many of the asserts are for assert foo is None which is a strict (albeit small) improvement over an equality check. Many others are for regex boolean checks which are really non-idiomatic in the status quo. For the other cases, I'll keep your comment in mind for future branches.

@ChadFulton
Copy link
Member

@jbrockmendel sorry about the delay, but TBH you make it somewhat difficult to consider whether or not to accept some of these PRs because you mix things like easy-to-commit pep8 improvements with opinion-based code changes like using the pytest.raises context.

I'm undecided here.

@jbrockmendel
Copy link
Contributor Author

You do you buddy. I just wanted to get this up so it doesn’t rot during my next leave of absence.

@ChadFulton
Copy link
Member

Just so you know where I'm coming from:

  • I would have accepted a pep8 improvement patch and / or one that replaced assert_equal with assert or assert_
  • I would have rejected a patch that removed assert_raises.

Since you don't seem to be in a compromising mood, we'll close this for now.

@ChadFulton ChadFulton closed this May 23, 2019
@jbrockmendel
Copy link
Contributor Author

Totally happy to compromise, will put together flake8-only version.

Out of curiosity, any idea why the temp_filename fixture in test_save is needed? Doesnt NamedTemporaryFile do exactly the same thing?

@jbrockmendel
Copy link
Contributor Author

in test_mlemodel.test_score_analytic_ar1 approx_fd_centered is defined but never used. im going to make the opinionated edit and decide it was intended to be part of the assertion in the following line

ChadFulton added a commit that referenced this pull request May 25, 2019
@bashtage bashtage added the rejected Used for PRs with changes that are not acceptable label Jul 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rejected Used for PRs with changes that are not acceptable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants