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

CLN/BUG: remove bare excepts #14554

Closed
jburroni opened this Issue Nov 1, 2016 · 7 comments

Comments

Projects
None yet
2 participants
@jburroni

jburroni commented Nov 1, 2016

Using KeyError may prevent the system to show system error, and thus make the error hard to find

df = pd.DataFrame({'a':[1,]}, index=['a'])
def bla(a):
    return bla(df.loc[['a'], ['a']])
bla(1)

Expected Output

Max recursion limit exception. Got key error

Output of pd.show_versions()

INSTALLED VERSIONS ------------------ commit: None python: 2.7.12.final.0 python-bits: 64 OS: Darwin OS-release: 15.6.0 machine: x86_64 processor: i386 byteorder: little LC_ALL: en_US.UTF-8 LANG: en_US.UTF-8

pandas: 0.18.1
nose: 1.3.7
pip: None
setuptools: None
Cython: None
numpy: 1.11.2
scipy: 0.18.1
statsmodels: None
xarray: None
IPython: 5.1.0
sphinx: None
patsy: None
dateutil: 2.5.3
pytz: 2016.7
blosc: None
bottleneck: None
tables: None
numexpr: None
matplotlib: 1.5.3
openpyxl: None
xlrd: None
xlwt: None
xlsxwriter: None
lxml: None
bs4: 4.4.1
html5lib: None
httplib2: None
apiclient: None
sqlalchemy: None
pymysql: None
psycopg2: None
jinja2: 2.8
boto: None
pandas_datareader: None

@jreback

This comment has been minimized.

Show comment
Hide comment
@jreback

jreback Nov 2, 2016

Contributor

what exactly is a KeyError?

This evaluates correctly to an infinite recursion.

Contributor

jreback commented Nov 2, 2016

what exactly is a KeyError?

This evaluates correctly to an infinite recursion.

@jburroni

This comment has been minimized.

Show comment
Hide comment
@jburroni

jburroni Nov 2, 2016

This is the KeyError (there was a little bug in the code snippet).
I'm using python 2.7
/../Library/Python/2.7/lib/python/site-packages/pandas/core/indexing.pyc in _multi_take(self, tup)
838 return o.reindex(**d)
839 except:
--> 840 raise self._exception
841
842 def _convert_for_reindex(self, key, axis=0):

KeyError: 

jburroni commented Nov 2, 2016

This is the KeyError (there was a little bug in the code snippet).
I'm using python 2.7
/../Library/Python/2.7/lib/python/site-packages/pandas/core/indexing.pyc in _multi_take(self, tup)
838 return o.reindex(**d)
839 except:
--> 840 raise self._exception
841
842 def _convert_for_reindex(self, key, axis=0):

KeyError: 
@jreback

This comment has been minimized.

Show comment
Hide comment
@jreback

jreback Nov 2, 2016

Contributor

and?

Contributor

jreback commented Nov 2, 2016

and?

@jburroni

This comment has been minimized.

Show comment
Hide comment
@jburroni

jburroni Nov 2, 2016

That there is no key error. The exception should be "max recursion limit",
but it was hid by the indexer. This makes the situation very hard to debug

On Tuesday, November 1, 2016, Jeff Reback notifications@github.com wrote:

and?


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#14554 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AChv6gnd2sEqEckdZXhvrNYTDKsDKmYEks5q5-lJgaJpZM4Kmt1b
.

" To be is to do " ( Socrates )
" To be or not to be " ( Shakespeare )
" To do is to be " ( Sartre )
" Do be do be do " ( Sinatra )

jburroni commented Nov 2, 2016

That there is no key error. The exception should be "max recursion limit",
but it was hid by the indexer. This makes the situation very hard to debug

On Tuesday, November 1, 2016, Jeff Reback notifications@github.com wrote:

and?


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#14554 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AChv6gnd2sEqEckdZXhvrNYTDKsDKmYEks5q5-lJgaJpZM4Kmt1b
.

" To be is to do " ( Socrates )
" To be or not to be " ( Shakespeare )
" To do is to be " ( Sartre )
" Do be do be do " ( Sinatra )

@jreback

This comment has been minimized.

Show comment
Hide comment
@jreback

jreback Nov 2, 2016

Contributor

I am still not clear what exactly is the issue. you have a recursion issue. This pandas call returns a valid value. What do you think this should do?

Contributor

jreback commented Nov 2, 2016

I am still not clear what exactly is the issue. you have a recursion issue. This pandas call returns a valid value. What do you think this should do?

@jburroni

This comment has been minimized.

Show comment
Hide comment
@jburroni

jburroni Nov 2, 2016

Part of the issue is in here:
https://github.com/pandas-dev/pandas/blob/master/pandas/core/indexing.py#L55
where the default exception is set to KeyError
Then, in
https://github.com/pandas-dev/pandas/blob/master/pandas/core/indexing.py#L850
all exception are caught and converted to KeyError. In the example above, the previous line will rise a RuntimeError with 'maximum recursion depth exceeded' but it will be caught and transformed in KeyError, which is incorrect (and misleading).
There are 6 extra 'except:' in that file. (And those are not recommended)

jburroni commented Nov 2, 2016

Part of the issue is in here:
https://github.com/pandas-dev/pandas/blob/master/pandas/core/indexing.py#L55
where the default exception is set to KeyError
Then, in
https://github.com/pandas-dev/pandas/blob/master/pandas/core/indexing.py#L850
all exception are caught and converted to KeyError. In the example above, the previous line will rise a RuntimeError with 'maximum recursion depth exceeded' but it will be caught and transformed in KeyError, which is incorrect (and misleading).
There are 6 extra 'except:' in that file. (And those are not recommended)

@jreback

This comment has been minimized.

Show comment
Hide comment
@jreback

jreback Nov 2, 2016

Contributor

sure. bare excepts are not recommended
submitting a pr is the best way to fix

Contributor

jreback commented Nov 2, 2016

sure. bare excepts are not recommended
submitting a pr is the best way to fix

@jreback jreback added this to the Next Major Release milestone Nov 2, 2016

@jreback jreback changed the title from _NDFrameIndexer should not use KeyError as default exception to CLN/BUG: remove bare excepts Nov 2, 2016

@clham clham referenced this issue Nov 20, 2016

Closed

CLN: remove bare excepts #14700

3 of 4 tasks complete

@jreback jreback modified the milestones: 0.19.2, Next Major Release Dec 19, 2016

@jreback jreback closed this in 3ccb501 Dec 19, 2016

jorisvandenbossche added a commit to jorisvandenbossche/pandas that referenced this issue Dec 24, 2016

CLN: Resubmit of GH14700. Fixes GH14554. Errors other than Indexing…
IdnexError and KeyError now bubble up appropriately.

closes #14554

Author: Chris Ham <chris@christopher-ham.com>

Closes #14912 from clham/gh14554-b and squashes the following commits:

458c0cc [Chris Ham] CLN: Resubmit of GH14700.  Fixes GH14554.  Errors other than IndexingError and KeyError now bubble up appropriately.

(cherry picked from commit 3ccb501)

ShaharBental added a commit to ShaharBental/pandas that referenced this issue Dec 26, 2016

CLN: Resubmit of GH14700. Fixes GH14554. Errors other than Indexing…
IdnexError and KeyError now bubble up appropriately.

closes #14554

Author: Chris Ham <chris@christopher-ham.com>

Closes #14912 from clham/gh14554-b and squashes the following commits:

458c0cc [Chris Ham] CLN: Resubmit of GH14700.  Fixes GH14554.  Errors other than IndexingError and KeyError now bubble up appropriately.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment