-
Notifications
You must be signed in to change notification settings - Fork 104
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
Avoid deprecated pandas is_categorical_dtype
API
#204
Conversation
patsy/util.py
Outdated
_pandas_is_categorical_dtype = None | ||
else: | ||
_pandas_is_categorical_dtype = lambda x: isinstance(x, pandas.CategoricalDType) | ||
_pandas_is_categorical_dtype = lambda x: isinstance(x, pandas.CategoricalDType) |
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.
Pretty sure this will not work on old pandas, e.g., those that work with python 2.7
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.
Hmmm... I presume this was for an older commit, since this should work pretty universally as is. I cannot remember exactly what the old code looked like, but I'm going to assume you were right :).
patsy/util.py
Outdated
@@ -45,18 +45,13 @@ | |||
_pandas_is_categorical_dtype = None | |||
else: | |||
if hasattr(pandas, "api"): |
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.
I think you could start with hasattr(pandas, "CategoricalDType")
and then the lambda
Failing this, you could do elif hasattr(pandas, "api")
, and then _pandas_is_categorical_dtype = getattr(pandas.api.types, "is_categorical_dtype", None)
,
finally, else, None
.
Something like
# default value
_pandas_is_categorical_dtype = None
if hasattr(pandas, "CategoricalDType"):
_pandas_is_categorical_dtype = lambda x: isinstance(x, pandas.CategoricalDType)
elif hasattr(pandas, "api") and getattr(pandas.api.types, "is_categorical_dtype", None) is not None:
def silenced_is_categorigal_type(typ):
with warnings.catch_warnings():
warnings.filterwarnings('ignore')
return pandas.api.types.is_categorical_dtype(typ)
_pandas_is_categorical_dtype = silenced_is_categorigal_type
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.
Yeah... still fiddling with this a bit... delving into pandas ancient history and seeing where things were introduced.
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.
Why do we need the warnings filter?
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.
I reckon just throw it in to make sure noting comes up. We know if is there then it will either FutureWarn, DepreactionWarn, or nothing. The first two are just noise.
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.
The current code looks like:
have_pandas_categorical = (have_pandas and hasattr(pandas, "Categorical"))
if not have_pandas:
_pandas_is_categorical_dtype = None
else:
if hasattr(pandas, "CategoricalDtype"): # pandas >= 0.25
_pandas_is_categorical_dtype = lambda x: isinstance(getattr(x, "dtype", x), pandas.CategoricalDtype)
elif hasattr(pandas, "api"): # pandas >= 0.19
_pandas_is_categorical_dtype = getattr(pandas.api.types, "is_categorical_dtype", None)
else: # pandas <=0.18
_pandas_is_categorical_dtype = getattr(pandas.core.common,
"is_categorical_dtype", None)
have_pandas_categorical_dtype = _pandas_is_categorical_dtype is not None
I don't see how we could ever generate a deprecation warning... so I'll just drop the warnings filter for now. It's arcane enough as it is :).
c00a420
to
713c585
Compare
713c585
to
f23e3bc
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #204 +/- ##
==========================================
+ Coverage 98.37% 98.46% +0.09%
==========================================
Files 30 30
Lines 3140 3133 -7
Branches 695 693 -2
==========================================
- Hits 3089 3085 -4
+ Misses 26 25 -1
+ Partials 25 23 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
This patch side-steps the issues addressed by #198 and #199, but avoiding recently deprecated APIs.