CLN GH22873 Replace base excepts in pandas/core

[pambot]

This cleaning fix adds specific exception classes to bare except statements to correct PEP8 linting errors. The classes were chosen by a mixture of educated guessing (i.e. if a statement gets the length property, it might raise an AttributeError), trying out functions on known edge cases (i.e. giving a datetime input that is too large gives tslibs.OutOfBoundsDatetime), and inspecting functions in the try block to see what kind of errors they raise.

• xref Replace bare excepts by explicit excepts in pandas/core/ #22873
• tests added / passed
• passes git diff upstream/master -u -- "*.py" | flake8 --diff
• whatsnew entry

[pep8speaks]

Hello @pambot! Thanks for updating the PR.

• There are no PEP8 issues in the file pandas/core/computation/pytables.py !

• There are no PEP8 issues in the file pandas/core/dtypes/common.py !

• There are no PEP8 issues in the file pandas/core/dtypes/dtypes.py !

• There are no PEP8 issues in the file pandas/core/frame.py !

• There are no PEP8 issues in the file pandas/core/indexes/frozen.py !

• There are no PEP8 issues in the file pandas/core/indexes/multi.py !

• There are no PEP8 issues in the file pandas/core/indexing.py !

• There are no PEP8 issues in the file pandas/core/internals/blocks.py !

• There are no PEP8 issues in the file pandas/core/nanops.py !

• There are no PEP8 issues in the file pandas/core/ops.py !

• There are no PEP8 issues in the file pandas/core/sparse/array.py !

• There are no PEP8 issues in the file pandas/core/tools/datetimes.py !

• There are no PEP8 issues in the file pandas/core/window.py !

Comment last updated on October 02, 2018 at 00:57 Hours UTC

[datapythonista]

The PR looks just great. There are couple of cases that I'm unsure, and would like to know your reasons for a change. Other than that, looks really good. Thanks for the hard work on this!

[datapythonista]

Thanks for adding this, but not sure if it's worth adding this change to the release note. It's more of a internal change that should not have any impact on users.

[pambot]

That's fine, I actually didn't think it was necessary too. I was just following the checklist.

[datapythonista]

Can you clarify this change? It feels like we just wanted to ignore KeyError exceptions, and I'm sure you have a reason to also ignore IndexError and ValueError, but it's not obvious to me.

[pambot]

No, you're right, I should revert it.

[datapythonista]

I don't understand this change either. Seems like the original behavior was to try to cast to float. Why to not try it anymore and raise directly?

[pambot]

@datapythonista My build is failing on origin/master, and I tried pulling from upstream just now, but it's still an issue. This could be something that is just happening to me, but I'm not sure what to do about it?

Specifically the tests are in pandas/tests/plotting/test_datetimelike.py and pandas/tests/io/test_parquet.py

[datapythonista]

If I'm not wrong, those were problems with the build unrelated to your PR. I think the datetime one is now fix in master, and should be ok if you merge the last master. The parquet one I think it was because of an amazon url, not sure if that's fixed, or it happens just some times. We need to take a look at it, don't worry much about it, if your changes are all right, we'll fix it eventually and merge your PR.

[codecov]

Codecov Report

Merging #22901 into master will decrease coverage by <.01%.
The diff coverage is 59.37%.

@@            Coverage Diff             @@
##           master   #22901      +/-   ##
==========================================
- Coverage   92.19%   92.18%   -0.01%     
==========================================
  Files         169      169              
  Lines       50833    50834       +1     
==========================================
- Hits        46863    46861       -2     
- Misses       3970     3973       +3

Flag	Coverage Δ
#multiple	90.6% <59.37%> (-0.01%)	⬇️
#single	42.37% <15.62%> (ø)	⬆️

Impacted Files	Coverage Δ
pandas/core/sparse/array.py	91.38% <0%> (ø)	⬆️
pandas/core/window.py	96.4% <0%> (ø)	⬆️
pandas/core/indexes/frozen.py	91.42% <0%> (ø)	⬆️
pandas/core/computation/pytables.py	92.37% <0%> (ø)	⬆️
pandas/core/dtypes/dtypes.py	95.85% <100%> (-0.26%)	⬇️
pandas/core/dtypes/common.py	95.02% <100%> (ø)	⬆️
pandas/core/ops.py	97.37% <100%> (ø)	⬆️
pandas/core/frame.py	97.2% <50%> (ø)	⬆️
pandas/core/nanops.py	95.14% <50%> (ø)	⬆️
pandas/core/internals/blocks.py	93.85% <60%> (ø)	⬆️
... and 4 more

---

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 5ce06b5...ae3d430. Read the comment docs.

[jreback]

where is this assertion error coming from?

[pambot]

Maybe we shouldn't be catching this, but since it was originally supposed to catch all the things, I thought it might have been designed to catch this in Series.__init__():

                if index is None:
                    index = data.index
                elif not data.index.equals(index) or copy:
                    # GH#19275 SingleBlockManager input should only be called
                    # internally
                    raise AssertionError('Cannot pass both SingleBlockManager '
                                         '`data` argument and a different '
                                         '`index` argument.  `copy` must '
                                         'be False.')

[jreback]

hmm, does removing AssertionError cause any tests to fail?

[jreback]

can we be more specific here?

[pambot]

The tests don't trigger if I remove this catch-all, which led to think that it was maybe put in because someone got an error once that they couldn't explain and this was the fix somehow? I kept this in for backwards compatibility, and the same for the Exception just below it.

[pambot]

Though if you tell me I should just put something anyway, I might put in (IndexError, ValueError) because of the slicing and the droplevel. On second thought, I'll just put this fix in anyway.

[jreback]

same here

[jreback]

can be more specific here

[pambot]

This is also the same as above, since I could only see TypeError being raised, but I'll replace it with except TypeError:

[jreback]

can be more specific here

[pambot]

I could be wrong here as well, but the reason why I put Exception here is because of the context, which was to enable the following behaviour:

except Exception:
    if errors == 'raise':
        raise

When I run the tests without this block, nothing gets raised, and I think if I try to guess which ones could get raised and it turns out that ones I didn't guess get raised during a user's runtime, then only the ones I guessed would get silenced by errors not being set to raise.

[jreback]

ok add a # noqa: E722

[jreback]

hmm, does removing AssertionError cause any tests to fail?

[jreback]

ok add a # noqa: E722

[datapythonista]

lgtm, and the problems with the build are fixed.

Thanks for taking care of this @pambot

[pambot]

@jreback I addressed your comments - could you please take another look?

[jreback]

thanks @pambot very nice!

[TomAugspurger]

This seems to be the first commit with the failure on numpydev. https://travis-ci.org/pandas-dev/pandas/jobs/435916255#L2767

____________ TestSelection.test_groupby_duplicated_column_errormsg _____________
[gw0] linux -- Python 3.7.0 /home/travis/miniconda3/envs/pandas/bin/python
self = <pandas.tests.groupby.test_grouping.TestSelection object at 0x7f87673fb668>
    def test_groupby_duplicated_column_errormsg(self):
        # GH7511
        df = DataFrame(columns=['A', 'B', 'A', 'C'],
                       data=[range(4), range(2, 6), range(0, 8, 2)])
    
>       pytest.raises(ValueError, df.groupby, 'A')
pandas/tests/groupby/test_grouping.py:42: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
pandas/core/generic.py:7136: in groupby
    observed=observed, **kwargs)
pandas/core/groupby/groupby.py:1966: in groupby
    return klass(obj, by, **kwds)
pandas/core/groupby/groupby.py:365: in __init__
    mutated=self.mutated)
pandas/core/groupby/grouper.py:588: in _get_grouper
    if is_categorical_dtype(gpr) and len(gpr) != obj.shape[axis]:
pandas/core/dtypes/common.py:574: in is_categorical_dtype
    return CategoricalDtype.is_dtype(arr_or_dtype)
pandas/core/dtypes/base.py:93: in is_dtype
    return cls.construct_from_string(dtype) is not None
pandas/core/dtypes/dtypes.py:359: in construct_from_string
    if string == 'category':
pandas/core/ops.py:1885: in f
    try_cast=False)
pandas/core/frame.py:4927: in _combine_const
    return ops.dispatch_to_series(self, other, func)
pandas/core/ops.py:1678: in dispatch_to_series
    new_data = expressions.evaluate(column_op, str_rep, left, right)
pandas/core/computation/expressions.py:205: in evaluate
    return _evaluate(op, op_str, a, b, **eval_kwargs)
pandas/core/computation/expressions.py:65: in _evaluate_standard
    return op(a, b)
pandas/core/ops.py:1658: in column_op
    for i in range(len(a.columns))}
pandas/core/ops.py:1658: in <dictcomp>
    for i in range(len(a.columns))}
pandas/core/ops.py:1510: in wrapper
    res = na_op(values, other)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
x = array([0, 2, 0]), y = 'category'
    def na_op(x, y):
        # TODO:
        # should have guarantess on what x, y can be type-wise
        # Extension Dtypes are not called here
    
        # Checking that cases that were once handled here are no longer
        # reachable.
        assert not (is_categorical_dtype(y) and not is_scalar(y))
    
        if is_object_dtype(x.dtype):
            result = _comp_method_OBJECT_ARRAY(op, x, y)
    
        elif is_datetimelike_v_numeric(x, y):
            return invalid_comparison(x, y, op)
    
        else:
    
            # we want to compare like types
            # we only want to convert to integer like if
            # we are not NotImplemented, otherwise
            # we would allow datetime64 (but viewed as i8) against
            # integer comparisons
    
            # we have a datetime/timedelta and may need to convert
            assert not needs_i8_conversion(x)
            mask = None
            if not is_scalar(y) and needs_i8_conversion(y):
                mask = isna(x) | isna(y)
                y = y.view('i8')
                x = x.view('i8')
    
            method = getattr(x, op_name, None)
            if method is not None:
                with np.errstate(all='ignore'):
>                   result = method(y)
E                   FutureWarning: elementwise comparison failed; returning scalar instead, but in the future will perform elementwise comparison

[TomAugspurger]

I think this may have been it. For whatever reason, we're here with

ipdb> pp string
   A  A
0  0  2
1  2  4
2  0  4

We do

ipdb> string == 'category'
*** FutureWarning: elementwise comparison failed; returning scalar instead, but in the future will perform elementwise comparison

That FutureWarning is elevated to an error. I didn't realize this, but you can catch warnings, so the very simple fix is to also catch FutureWarning here, but that feels fragile. I'll investigate a better fix...

[TomAugspurger]

Moving further discussion to #22960