Skip to content

ARROW-3953: [Python] Compat with pandas 0.24 rename of MultiIndex labels -> codes #3120

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

Conversation

jorisvandenbossche
Copy link
Member

No description provided.

@jorisvandenbossche
Copy link
Member Author

I am not fully sure if I should add a test for this. The result of this patch is that there should be no warnings about the deprecation, so I could add an assert that there are no warnings in one of the tests?

Is pandas master tested only in a nightly test build? Or also in travis?

@codecov-io
Copy link

Codecov Report

Merging #3120 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3120      +/-   ##
==========================================
- Coverage   87.09%   87.07%   -0.02%     
==========================================
  Files         492      492              
  Lines       69160    69164       +4     
==========================================
- Hits        60233    60226       -7     
- Misses       8830     8837       +7     
- Partials       97      101       +4
Impacted Files Coverage Δ
python/pyarrow/pandas_compat.py 98.01% <100%> (+0.02%) ⬆️
go/arrow/math/int64_avx2_amd64.go 0% <0%> (-100%) ⬇️
go/arrow/memory/memory_avx2_amd64.go 0% <0%> (-100%) ⬇️
go/arrow/math/float64_avx2_amd64.go 0% <0%> (-100%) ⬇️
go/arrow/math/uint64_avx2_amd64.go 0% <0%> (-100%) ⬇️
go/arrow/memory/memory_amd64.go 28.57% <0%> (-14.29%) ⬇️
go/arrow/math/math_amd64.go 31.57% <0%> (-5.27%) ⬇️
cpp/src/plasma/thirdparty/ae/ae.c 72.03% <0%> (-0.95%) ⬇️
go/arrow/math/float64_amd64.go 33.33% <0%> (ø) ⬆️
go/arrow/math/int64_amd64.go 33.33% <0%> (ø) ⬆️
... and 6 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 072df89...329f3e4. Read the comment docs.

@wesm
Copy link
Member

wesm commented Dec 7, 2018

I think we have a docker-compose target for testing against pandas master. cc @kszucs

@xhochy
Copy link
Member

xhochy commented Dec 9, 2018

These instructions for the pandas tests can be found at

arrow/docker-compose.yml

Lines 246 to 252 in 5704d8d

pandas-master:
# Usage:
# export PYTHON_VERSION=3.6
# docker-compose build cpp
# docker-compose build python
# docker-compose build --no-cache pandas-master
# docker-compose run pandas-master

@kszucs
Copy link
Member

kszucs commented Dec 9, 2018

@jorisvandenbossche a test case would be nice (BTW the warning indeed disappears, tested with the docker-compose snippet above)

@jorisvandenbossche
Copy link
Member Author

Are the docker-compose / crossbow tests also run somewhere on CI, or are you supposed to run them locally? (if I add a test, it could only fail on pandas master, so it wouldn't be catched on travis I think?)

@kszucs
Copy link
Member

kszucs commented Dec 10, 2018

@jorisvandenbossche You can make the test case dependent on pandas' version

@pitrou
Copy link
Member

pitrou commented Dec 12, 2018

I'm not sure it's worth writing a test for this, TBH.

@jorisvandenbossche
Copy link
Member Author

Sorry, I will be mainly on holidays the comings weeks, so won't be able to add a test until after (I had some problems compiling yesterday I first need to solve).

A test would be something like adding the following assert to one of the existing tests:

with pytest.warns(None) as record:
    # existing conversion from arrow table to pandas
    ...
# assert pandas raises no FutureWarning about codes being deprecated
assert len([w for w in record if w.category is FutureWarning and 'labels' in str(w.message)]) == 0

I think it can be nice to add it to make sure we don't introduce a usage of labels again, but it's certainly not essential, because the fact that tests are passing both on released pandas and pandas master is the actual test for the functionality.

@xhochy
Copy link
Member

xhochy commented Dec 14, 2018

I would also be happy merging this without a test, @kszucs do you feel strongly about this and could add one?

@kszucs
Copy link
Member

kszucs commented Dec 15, 2018

Not necessarily, but if order is the soul of everything then testing is the soul of engineering

Copy link
Member

@kszucs kszucs left a comment

Choose a reason for hiding this comment

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

@kszucs kszucs closed this in 1fd2a25 Dec 15, 2018
@jorisvandenbossche jorisvandenbossche deleted the pandas-multiindex-codes branch December 16, 2018 07:17
cav71 pushed a commit to cav71/arrow that referenced this pull request Dec 22, 2018
…els -> codes

Author: Joris Van den Bossche <[email protected]>
Author: Krisztián Szűcs <[email protected]>

Closes apache#3120 from jorisvandenbossche/pandas-multiindex-codes and squashes the following commits:

e5442a5 <Krisztián Szűcs> test no warns
329f3e4 <Joris Van den Bossche> Compat with pandas 0.24 rename of MultiIndex labels -> codes
@jorisvandenbossche
Copy link
Member Author

@kszucs Thanks for adding the test!

@kszucs
Copy link
Member

kszucs commented Jan 3, 2019

You're welcome :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants