BUG: not raising error on Newey-West corrections when clustering #8170
Conversation
| if (kwargs.get('cluster') is not None and | ||
| kwargs.get('nw_lags') is not None): | ||
| raise AssertionError( | ||
| 'OLS does not work with Newey-West correction ' |
|
jreback
changed the title
|
The assertion I added (which I'll change to a ValueError and add a test for, as requested), has caused several other tests that run OLS with clustering and Newey-West adjustments to fail. What's the recommended course of action to get all the tests to pass again? Raise nose.SkipTest() on the problematic tests? Have them test for the ValueError instead of whatever they were originally testing for? |
|
if other tests look for the AssertionError then change them. Generally need to be very careful about changing tests as could indicate an actual problem) |
|
Sorry, my comment was unclear. What I meant was that some pre-existing tests, such as pandas.stats.tests.test_ols.TestPanelOLS.testRollingWithNeweyWestAndEntityCluster, fail because they try to run OLS with Newey-West and clustering, so they fail due to the new error check that I'm adding in this pull request. Because my change causes OLS to error anytime Newey-West and clustering are both used, any tests that do this will now fail. This is the issue that I'm unsure how to resolve. |
|
are these valid ways of using it? if not then the tests should fail (so change the test to to assertRaises) |
|
No, using OLS with clustering and Newey-West does not work properly, per GH5884. I'll change the affected tests to use assertRaises. Thanks for the help! |
|
Changed the AssertionError to a ValueError, and changed the two broken tests to assert that the ValueError is raised when OLS is called with Newey-West and clustering. |
|
excellent! pls add a release note in v0.15.0.txt (you can put it in Bug Fixes) ping when green |
|
Unfortunately, I was away for a week and removed my changes from this pull request, planning to sync to head and then reapply when I got back. As a result, when this pull request was closed and merged, it appears that my changes did not actually make it into the codebase. I'll start another pull request; since I'm familiar with the procedure this time, I'll make sure all the changes are in one commit and all the tests pass, so it should be a simple, quick merge this time. Sorry for the trouble; this was my first attempt at a pull request on Github. |
|
@jessefarnham hmm, this is very odd. Looks like an old github bug that does this. Ok pls resubmit and then will get it in. |
closes #5884, OLS does not correctly apply Newey-West corrections when clustering
is being used. Add an assertion to check for this.