CLN: remove bare excepts #14700

clham · 2016-11-20T13:50:40Z

closes CLN/BUG: remove bare excepts #14554
tests added / passed: updated test_indexing.py
passes git diff upstream/master | flake8 --diff
whatsnew entry

Errors in indexing no longer forced into KeyError

codecov-io · 2016-11-20T15:43:20Z

Current coverage is 84.55% (diff: 0.00%)

Merging #14700 into master will not change coverage

@@             master     #14700   diff @@
==========================================
  Files           144        144          
  Lines         51043      51043          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits          43160      43160          
  Misses         7883       7883          
  Partials          0          0

Powered by Codecov. Last update 73bc6cf...6c4b4cd

jreback · 2016-11-21T11:26:30Z

pandas/core/indexing.py

@@ -1320,8 +1320,8 @@ def _getbool_axis(self, key, axis=0):
        inds, = key.nonzero()
        try:
            return self.obj.take(inds, axis=axis, convert=False)
-        except Exception as detail:


instead of this, catch appropriate errors (and let others bubble up), and convert to the correct exception type. as written this is a big change in the API

jreback · 2016-11-21T11:26:43Z

pandas/tests/indexing/test_indexing.py

@@ -1216,7 +1216,7 @@ def test_loc_getitem_bool(self):
        self.check_result('bool', 'loc', b, 'ix', b,
                          typs=['ints', 'labels', 'mixed', 'ts', 'floats'])
        self.check_result('bool', 'loc', b, 'ix', b, typs=['empty'],
-                          fails=KeyError)


this is a major API change, pls revert

jreback · 2016-11-21T11:26:55Z

needs the test from the issue

jreback

please make the changes as indicated

clham · 2016-11-22T12:32:35Z

I'm struggling with the intent of what's going on then.

Indexing errors and key errors should be converted to key error, everything else (like the bug reports recursion error) shouldbubble up? Thanks for the understanding, This is one of my first non doc PRs.

jreback · 2016-11-22T23:43:24Z

Indexing errors and key errors should be converted to key error, everything else (like the bug reports recursion error) shouldbubble up? Thanks for the understanding, This is one of my first non doc PRs.

no they should be converted to the _exception. you can try just catching IndexError/TypeError where where the bare exceptions are now.

Its very important that the exceptions that are related to indexing are consistent (think from a user perspective). and so we don't/can't change them. However we may have an implementation that actually does positional type things (e.g. .take), which may raise an error for something not found, but (in this case) are doing label indexing so a KeyError is correct, but the same code block may also handle positional indexing (the subclasses define _exception), so the _exception is then IndexError (again for consistency).

jreback · 2016-12-05T23:49:01Z

pandas/core/indexing.py

            raise self._exception
+        except:


you don't need an except here (it will default to raising).

you also don't need to catch self._exception

jreback · 2016-12-05T23:49:27Z

pandas/tests/indexing/test_indexing.py

@@ -1218,6 +1222,18 @@ def test_loc_getitem_bool(self):
        self.check_result('bool', 'loc', b, 'ix', b, typs=['empty'],
                          fails=KeyError)

+    def test_recursion_fails_loc(self):
+        #GH14554


you need a space after the comments

jreback · 2016-12-05T23:49:57Z

pandas/tests/indexing/test_indexing.py

@@ -279,6 +278,11 @@ def _print(result, error=None):
                        k2 = key2
                        _eq(t, o, a, obj, key1, k2)

+    #GH14554: used to make sure recursion errors bubble up as expected
+    recursive_df = pd.DataFrame({'a':[1,]}, index=['a'])
+    def recursive_indexing_function(self, a):


can we define this inside of the test function?

clham · 2016-12-15T02:40:56Z

@jreback What's the best course of action here? Some of the builds are failing, but for conflicting reasons. 3.4 is blowing the stack, 3.5 is failing because it because the recursion limit is being set to low. I've reached an impasse in googling. Something doesn't smell right in artificially setting the setrecursionlimit, but I can't think of another way to ensure the stack doesn't actually overflow before the exception it thrown?

jreback · 2016-12-15T11:17:37Z

pandas/tests/indexing/test_indexing.py

+            return recursive_indexing_function(
+                recursive_df[['a'], ['a']])
+
+        oldVal=sys.getrecursionlimit()


why are you changing the recursion limit at all?
just let the assert happen naturally.

If I exclude it, when I run the tests, my stack just blows up, and the exception doesn't bubble up.

yeah this seems kind of extreme. why don't you just remove this test and leave your change. pls add a whatsnew note (for 0.20.0), and we'll call it a day :>

of course give it a manual test that things work

clham · 2016-12-17T02:25:44Z

@jreback Updated and squashed.

jreback · 2016-12-17T14:39:12Z

can you rebase and push again. like to see this all green.

…dexError

clham · 2016-12-17T23:50:23Z

My git-foo was all jacked up. I have no idea what happened. Please let me know if current push works.

jorisvandenbossche · 2016-12-17T23:54:47Z

@clham Can you push again? For some reason, the tests didn't run

clham · 2016-12-18T00:00:00Z

Done. Still seems not to be triggering. Should I try adding a commit?

jorisvandenbossche · 2016-12-18T00:03:19Z

Can you fix the merge conflict and in the whatsnew file, and push that?

clham · 2016-12-18T00:39:07Z

made a change and repushed: still doesn't appear to be getting pulled into Travis or AppVeyor. Conflict was also resolved. it is only a few small changes. does it make sens to nuke the branch and submit a fresh PR?

clham · 2016-12-18T21:41:47Z

resubmitted as #14912 . Just starting over to make the pain stop.

jreback reviewed Nov 21, 2016

View reviewed changes

jreback changed the title ~~BUG: Fixes GH14554~~ CLN: remove bare excepts Nov 21, 2016

jreback added Clean Error Reporting Incorrect or improved errors from pandas labels Nov 21, 2016

jreback requested changes Nov 22, 2016

View reviewed changes

jreback reviewed Dec 5, 2016

View reviewed changes

jreback reviewed Dec 15, 2016

View reviewed changes

clham force-pushed the gh14554 branch from cf505db to 1ecc540 Compare December 17, 2016 02:20

jreback approved these changes Dec 17, 2016

View reviewed changes

closes GH14554, RecursionError nolonger coerced into a KeyError or In…

ef115bd

…dexError

clham force-pushed the gh14554 branch from 6c4b4cd to ef115bd Compare December 17, 2016 20:24

quick edit for force a change

1844b83

jreback closed this Dec 19, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CLN: remove bare excepts #14700

CLN: remove bare excepts #14700

clham commented Nov 20, 2016 •

edited

Loading

codecov-io commented Nov 20, 2016 •

edited

Loading

jreback Nov 21, 2016

jreback Nov 21, 2016

jreback commented Nov 21, 2016

jreback left a comment

clham commented Nov 22, 2016

jreback commented Nov 22, 2016

jreback Dec 5, 2016

jreback Dec 5, 2016

jreback Dec 5, 2016

clham commented Dec 15, 2016

jreback Dec 15, 2016

clham Dec 16, 2016

jreback Dec 16, 2016

clham commented Dec 17, 2016

jreback commented Dec 17, 2016

clham commented Dec 17, 2016

jorisvandenbossche commented Dec 17, 2016

clham commented Dec 18, 2016

jorisvandenbossche commented Dec 18, 2016

clham commented Dec 18, 2016

clham commented Dec 18, 2016 •

edited

Loading

CLN: remove bare excepts #14700

CLN: remove bare excepts #14700

Conversation

clham commented Nov 20, 2016 • edited Loading

codecov-io commented Nov 20, 2016 • edited Loading

Current coverage is 84.55% (diff: 0.00%)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Nov 21, 2016

jreback left a comment

Choose a reason for hiding this comment

clham commented Nov 22, 2016

jreback commented Nov 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clham commented Dec 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clham commented Dec 17, 2016

jreback commented Dec 17, 2016

clham commented Dec 17, 2016

jorisvandenbossche commented Dec 17, 2016

clham commented Dec 18, 2016

jorisvandenbossche commented Dec 18, 2016

clham commented Dec 18, 2016

clham commented Dec 18, 2016 • edited Loading

clham commented Nov 20, 2016 •

edited

Loading

codecov-io commented Nov 20, 2016 •

edited

Loading

clham commented Dec 18, 2016 •

edited

Loading