-
Notifications
You must be signed in to change notification settings - Fork 34
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
Fix test failures with Pandas 1.2.0 #157
Conversation
There seem to be a number of different issue with Pandas 1.2.0, I'll look into fixing them here too. |
@BryanCutler any idea what's causing that test failure on the Python 3.7 build (https://github.com/CODAIT/text-extensions-for-pandas/pull/157/checks?check_run_id=1620791768)? |
I fixed most of the issues with pandas 1.2.0 and TensorArray, except there is a regression with |
_get_op_name()
0e34568
to
caee6a7
Compare
@@ -445,6 +446,8 @@ def test_make_exploded_df(self): | |||
15 Total tax rate \ | |||
""") | |||
|
|||
@pytest.mark.skipif(LooseVersion(pd.__version__) >= LooseVersion("1.2.0"), | |||
reason="TODO: Rank col gets converted to float") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why this is different now, but I think it's safe to skip and I can make an issue to follow up with later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works for me.
@frreiss this failure https://github.com/CODAIT/text-extensions-for-pandas/pull/157/checks?check_run_id=1646927381#step:5:1438 in |
Ok, I think I found why the above error is happening and looks to be a bug. Pandas attempts to do a cython aggregation and then fallback to a vanilla agg if there is an error, but checks the error message improperly for this case. I should be able to file a bug report and do a PR for the fix. |
Thanks for tracking down the root cause of that problem, @BryanCutler ! |
PR for the agg fix pandas-dev/pandas#38982 |
@frreiss I capped the upper version of Pandas to < 1.2.0 because not being able to display a Series with TensorArray of floats is pretty major and I don't see a workaround. I'll keep working on the upstream fix, but what are your thoughts on merging this? |
I think merging is the best way forward for now. We may want to post a new release just to get that pandas<1.2.0 constraint into our requirements.txt. |
There are a number of test failures when using Pandas 1.2.0.
pandas.core.ops._get_op_name()
has been removed in Pandas >=1.2.0. This was being used when buildingTensorArray
ops, and is replaced with similar usage inExtensionScalarOpsMixin
.TensorArray
should return it's instance whenastype()
called with same typeTensorArray._contains_
needs to have proper implementation ofany()
repr()
is broken when using float values of ndims > 2Fixes #158