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

get rid of CategoricalArrays #157

Merged
merged 8 commits into from
Nov 11, 2019
Merged

get rid of CategoricalArrays #157

merged 8 commits into from
Nov 11, 2019

Conversation

kleinschmidt
Copy link
Member

@kleinschmidt kleinschmidt commented Oct 2, 2019

Depends on having a levels method defined in DataAPI (or elsewhere), which it currently does not which it does since 1.1.0. Also, there were a few functions from Missings that snuck in because CategoricalArrays reexports it.

Depends on having a levels method defined in DataAPI, which it currently does
not.  Also, there were a few functions from Missings that snuck in because
CategoricalArrays reexports it.
@kleinschmidt
Copy link
Member Author

see JuliaData/DataAPI.jl#12

@kleinschmidt
Copy link
Member Author

This will also depend on Categorical Arrays pulling in levels from DataAPI instead of Missing

@kleinschmidt
Copy link
Member Author

kleinschmidt commented Oct 3, 2019

@nalimilan pointed out that we can still use the specialized _nonmissing! method to handle categorical data more efficiently if we use DataAPI.refarray (with the bonus that it also then would handle PooledArrays too)

res[i] &= el > 0
end
end
# TODO: find another way to optimize this without taking a dependency
Copy link
Member

Choose a reason for hiding this comment

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

Just remove this thanks to JuliaData/CategoricalArrays.jl#218.

Copy link
Member Author

Choose a reason for hiding this comment

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

does this mean that we need to change these methods to use broadcast instead of doing a loop?

@kleinschmidt
Copy link
Member Author

I think this is ready now that Missings, DataAPI, and CategoricalArrays have all had releases.

@kleinschmidt kleinschmidt marked this pull request as ready for review October 16, 2019 18:46
@codecov-io
Copy link

codecov-io commented Oct 16, 2019

Codecov Report

Merging #157 into master will decrease coverage by 0.09%.
The diff coverage is 75%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #157     +/-   ##
=========================================
- Coverage   84.87%   84.78%   -0.1%     
=========================================
  Files           9        9             
  Lines         496      493      -3     
=========================================
- Hits          421      418      -3     
  Misses         75       75
Impacted Files Coverage Δ
src/StatsModels.jl 100% <ø> (ø) ⬆️
src/modelframe.jl 87.8% <100%> (-0.84%) ⬇️
src/statsmodel.jl 86.44% <66.66%> (ø) ⬆️

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 6d581b1...b566f35. Read the comment docs.

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