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: fixed check _is_unorderable_exception #17724

Conversation

oleksandr-pavlyk
Copy link

With upgrade to NumPy 1.13.2 the error message raised when comparing unorderable types
changes from using "'>' not supported between instances of" to using
"'<' not supported between instances of".

This PR checks both of these.

See GH-17046 for discussion.

This caused failing test test_basic_indexing when running tests with NumPy 1.13.2.

Here is the reproducer

import pandas
import pytest
import numpy as np
def test_basic_indexing():
    s = pandas.Series(np.random.randn(5), index=['a', 'b', 'a', 'a', 'b'])

    pytest.raises(IndexError, s.__getitem__, 5)
    pytest.raises(IndexError, s.__setitem__, 5, 0)

    pytest.raises(KeyError, s.__getitem__, 'c')

    s = s.sort_index()

    pytest.raises(IndexError, s.__getitem__, 5)
    pytest.raises(IndexError, s.__setitem__, 5, 0) # this part was failing

test_basic_indexing()

@sinhrks sinhrks added the Error Reporting Incorrect or improved errors from pandas label Sep 29, 2017
@@ -1158,7 +1158,9 @@ def _is_unorderable_exception(e):
"""

if PY36:
return "'>' not supported between instances of" in str(e)
str_e = str(e)
return "'>' not supported between instances of" in str_e or \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Parenthesized multi-line strings are preferred in this codebase over slashes.

@oleksandr-pavlyk oleksandr-pavlyk force-pushed the feature/np1.13.2-related-failures branch from 9e21a58 to 02d3516 Compare October 1, 2017 13:02
@pep8speaks
Copy link

pep8speaks commented Oct 1, 2017

Hello @oleksandr-pavlyk! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on October 01, 2017 at 13:04 Hours UTC

@codecov
Copy link

codecov bot commented Oct 1, 2017

Codecov Report

Merging #17724 into master will decrease coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #17724      +/-   ##
==========================================
- Coverage   91.27%   91.25%   -0.03%     
==========================================
  Files         163      163              
  Lines       49766    49766              
==========================================
- Hits        45422    45412      -10     
- Misses       4344     4354      +10
Flag Coverage Δ
#multiple 89.04% <ø> (-0.01%) ⬇️
#single 40.33% <ø> (-0.07%) ⬇️
Impacted Files Coverage Δ
pandas/core/dtypes/common.py 94.22% <ø> (-0.24%) ⬇️
pandas/io/gbq.py 25% <0%> (-58.34%) ⬇️
pandas/core/frame.py 97.73% <0%> (-0.1%) ⬇️
pandas/core/dtypes/inference.py 98.33% <0%> (-0.03%) ⬇️
pandas/core/indexes/multi.py 96.9% <0%> (ø) ⬆️

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 baadad7...02d3516. Read the comment docs.

@codecov
Copy link

codecov bot commented Oct 1, 2017

Codecov Report

Merging #17724 into master will decrease coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #17724      +/-   ##
==========================================
- Coverage   91.27%   91.25%   -0.03%     
==========================================
  Files         163      163              
  Lines       49766    49766              
==========================================
- Hits        45422    45412      -10     
- Misses       4344     4354      +10
Flag Coverage Δ
#multiple 89.04% <ø> (-0.01%) ⬇️
#single 40.33% <ø> (-0.07%) ⬇️
Impacted Files Coverage Δ
pandas/core/dtypes/common.py 94.22% <ø> (-0.24%) ⬇️
pandas/io/gbq.py 25% <0%> (-58.34%) ⬇️
pandas/core/frame.py 97.73% <0%> (-0.1%) ⬇️
pandas/core/dtypes/inference.py 98.33% <0%> (-0.03%) ⬇️
pandas/core/indexes/multi.py 96.9% <0%> (ø) ⬆️

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 baadad7...855d03f. Read the comment docs.

With upgrade to NumPy 1.13.2 the error message raised when comparing unorderable types
changes from using "'>' not supported between instances of" to using
"'<' not supported between instances of".

This PR checks of these.

See pandas-devGH-17046 for discussion.

This caused failing test test_basic_indexing. Here is the reproducer

```
import pandas
import pytest
import numpy as np
def test_basic_indexing():
    s = pandas.Series(np.random.randn(5), index=['a', 'b', 'a', 'a', 'b'])

    pytest.raises(IndexError, s.__getitem__, 5)
    pytest.raises(IndexError, s.__setitem__, 5, 0)

    pytest.raises(KeyError, s.__getitem__, 'c')

    s = s.sort_index()

    pytest.raises(IndexError, s.__getitem__, 5)
    pytest.raises(IndexError, s.__setitem__, 5, 0) # this part was failing

test_basic_indexing()
```
@oleksandr-pavlyk oleksandr-pavlyk force-pushed the feature/np1.13.2-related-failures branch from 02d3516 to 855d03f Compare October 1, 2017 13:04
@jreback
Copy link
Contributor

jreback commented Oct 1, 2017

can you point to something specific which is actually failing with a minimal env.. We tests vs numpy master and this doesn't come up.

@jreback
Copy link
Contributor

jreback commented Oct 1, 2017

this issue you are referncing: #17051 is already been merged a while back

@oleksandr-pavlyk
Copy link
Author

@jreback Yes, the #17051 has merged, I was simply pointing to the similar problem. That PR was dealing with a fall-out from the same change in the different tests.

To reproduce this problem it is important to use numpy 1.13.2 or 1.13.3 while running the test.

Note that Travis uses NumPy 1.13.1 for testing, since miniconda does not ship NumPy 1.13.2 yet.

@jreback
Copy link
Contributor

jreback commented Oct 3, 2017

Note that Travis uses NumPy 1.13.1 for testing, since miniconda does not ship NumPy 1.13.2 yet.

@oleksandr-pavlyk you would have to prove that this doesn't work as we already test on the CI with numpy 1.13.3: https://travis-ci.org/pandas-dev/pandas/jobs/282520540 (this is an all conda-forge build), and with numpy master: https://travis-ci.org/pandas-dev/pandas/jobs/282520544

our other builds use defaults for the base build.

both of which pass.

sure the currently *released *version (IOW 0.20.3) would fail with 1.13.2 on these tests, but that's not relevant).

@oleksandr-pavlyk
Copy link
Author

Yes, my fix was developed with 0.20.3 in mind.

Regarding your comment as to the relevancy. It is relevant for Python distributors, like Anaconda, or Intel Distribution for Python, as customers generally expect distributions to have up-to-date packages.

@jreback
Copy link
Contributor

jreback commented Oct 3, 2017

Yes, my fix was developed with 0.20.3 in mind.

closing, this is already fixed in master.

Regarding your comment as to the relevancy. It is relevant for Python distributors, like Anaconda, or Intel Distribution for Python, as customers generally expect distributions to have up-to-date packages.

not what I meant. we don't backport from major releases. you can get 0.21.0 when it is out.

@jreback jreback closed this Oct 3, 2017
yarikoptic pushed a commit to neurodebian/pandas that referenced this pull request Feb 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Error Reporting Incorrect or improved errors from pandas
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants