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

BUG: Fix refactor victim. #1657

Merged
merged 2 commits into from May 6, 2014

Conversation

Projects
None yet
3 participants
@jseabold
Copy link
Member

commented May 6, 2014

Summary2 still doesn't have even minimal smoke tests, apparently. This snuck in during the 2to3 refactor.

cc @vincentarelbundock

@vincentarelbundock

This comment has been minimized.

Copy link
Contributor

commented May 6, 2014

Yes, you're right. I might have a bit of time to invest in this in the next few weeks, but my degree of effort will likely be contingent on whether there's a plan for this thing. Useless code with smoke tests is still useless ;)

@jseabold

This comment has been minimized.

Copy link
Member Author

commented May 6, 2014

I discovered it because I was trying to see whether summary2 fixed #1584 easily. If it does, then it's a pro for using it. No tests means not production ready, though.

@jseabold

This comment has been minimized.

Copy link
Member Author

commented May 6, 2014

The answer is no it doesn't, since this relies on SimpleTable anyway for the parameters table.

@josef-pkt

This comment has been minimized.

Copy link
Member

commented May 6, 2014

As TravisCI shows, there is the same smoke test for summary2() as for summary()

@jseabold

This comment has been minimized.

Copy link
Member Author

commented May 6, 2014

The problem then is that the repr swallowed the error, so I guess smoketests aren't enough.

mod.summary2()
[95]: <repr(<statsmodels.iolib.summary2.Summary at 0x2751c710>) failed: pandas.core.indexing.IndexingError: Too many indexers>

But printing

print(mod.summary2())
<snip>
IndexingError: Too many indexers
@josef-pkt

This comment has been minimized.

Copy link
Member

commented May 6, 2014

The problem was that the key error was inside a try ... except, and the exception is swallowed.
smoke tests are not enough for this

@jseabold

This comment has been minimized.

Copy link
Member Author

commented May 6, 2014

The problem was that the refactor changed .keys() to iteritems which yields tuples not a key.

This error being ignored is what caused the indexing error (the info ended up being empty), but it was never swallowed. It's only swallowed by repr as I showed above.

@josef-pkt

This comment has been minimized.

Copy link
Member

commented May 6, 2014

right, it caught AttributeError but not KeyError, I misread

@josef-pkt

This comment has been minimized.

Copy link
Member

commented May 6, 2014

summary2 has three blanket try ... except:

jseabold added a commit that referenced this pull request May 6, 2014

Merge pull request #1657 from jseabold/summary2-bug
BUG: Fix refactor victim.

@jseabold jseabold merged commit b472807 into statsmodels:master May 6, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@jseabold jseabold deleted the jseabold:summary2-bug branch May 6, 2014

@josef-pkt josef-pkt added PR labels May 21, 2014

PierreBdR pushed a commit to PierreBdR/statsmodels that referenced this pull request Sep 2, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.