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

Deprecate use of categorical! function on non-AbstractString columns #1254

Closed
wants to merge 1 commit into from

Conversation

cjprybol
Copy link
Contributor

Fixes #1250

@coveralls
Copy link

coveralls commented Oct 12, 2017

Coverage Status

Coverage increased (+0.1%) to 72.671% when pulling cefa51d on cjp/categoricalstringsonly into 8fd0851 on master.

@@ -740,20 +740,28 @@ end
##############################################################################

function categorical!(df::DataFrame, cname::Union{Integer, Symbol})
df[cname] = CategoricalVector(df[cname])
if Nulls.T(eltype(df[cname])) <: AbstractString
Copy link
Member

Choose a reason for hiding this comment

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

Instead of adding this deprecation here, I'd rather do that directly in CategoricalArrays. That way we have only one deprecation to remove once we decide the deprecation period is over.

@cjprybol
Copy link
Contributor Author

Waiting on (post-merge) discussions on what types to support in CategoricalArrays here -> JuliaData/CategoricalArrays.jl#77

@nalimilan
Copy link
Member

With JuliaData/CategoricalArrays.jl#87, I guess we can keep the current behavior which converts all columns to CategoricalArray?

@cjprybol
Copy link
Contributor Author

Yes! Very exciting to see JuliaData/CategoricalArrays.jl#87 get merged

@cjprybol cjprybol closed this Oct 24, 2017
@cjprybol cjprybol deleted the cjp/categoricalstringsonly branch October 24, 2017 03:21
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.

3 participants