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

Port to CategoricalArrays 0.8 #602

Merged
merged 5 commits into from
May 6, 2020
Merged

Port to CategoricalArrays 0.8 #602

merged 5 commits into from
May 6, 2020

Conversation

nalimilan
Copy link
Member

The change to levels! implies that all reference codes will be updated at the end to match the new order of levels (unless they were already sorted).

Requires the yet-to-be released CategoricalArrays 0.8.

The change to `levels!` implies that all reference codes will be updated
at the end to match the new order of levels (unless they were already sorted).
Copy link
Member

@quinnj quinnj left a comment

Choose a reason for hiding this comment

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

Looks great; love minimal changes.

@bkamins
Copy link
Member

bkamins commented Apr 20, 2020

Can it be merged then?

@nalimilan
Copy link
Member Author

nalimilan commented Apr 20, 2020

It needs DataFrames 0.21 to pass CI (EDIT: otherwise the dependencies cannot be satisfied).

@bkamins
Copy link
Member

bkamins commented Apr 20, 2020

So this means that we have to coordinate releasing DataFrames.jl and CSV.jl? In this case maybe in this PR you can bump DataFrames.jl version and CSV.jl version in Project.toml?

@nalimilan
Copy link
Member Author

Well we have to release DataFrames first, and then ideally CSV.jl soon after that. If we don't do it, people who have CSV installed will stay at DataFrames 0.20, which isn't the end of the world but is... annoying. :-) But we also have to port many other packages, like RCall.

@quinnj
Copy link
Member

quinnj commented Apr 21, 2020

I could do the next CSV release at any point now. I'm tracking down loose ends and kicking the tires on more speculative features; and I'm going to try to do another thorough review of performance/memory. But nothing release blocking. So I'll let @nalimilan drive a coordinated release for CategoricalArrays/DataFrames and CSV can tag along.

@quinnj
Copy link
Member

quinnj commented Apr 28, 2020

@nalimilan, should we merge here and do a new release?

@bkamins
Copy link
Member

bkamins commented Apr 28, 2020

We need to sync this with DataFrames.jl release.

@quinnj
Copy link
Member

quinnj commented Apr 28, 2020

Ah right; I think in my mind, I was worried we were holding the DataFrames release because of this, but I suspect @nalimilan will known when to merge and then we can tag.

@bkamins
Copy link
Member

bkamins commented Apr 28, 2020

The releases must be synced. We need to finish JuliaData/DataFrames.jl#2214 (complex) and JuliaData/DataFrames.jl#2196 (easy) and then we can make a release of DataFrames.jl.

@bkamins
Copy link
Member

bkamins commented May 5, 2020

Version of CSV.jl in Project.toml needs to be changed also (unless you want to do it in a separate PR)

@nalimilan nalimilan marked this pull request as ready for review May 5, 2020 17:30
@bkamins
Copy link
Member

bkamins commented May 5, 2020

CI looks good now - thank you!

@codecov
Copy link

codecov bot commented May 5, 2020

Codecov Report

Merging #602 into master will increase coverage by 1.66%.
The diff coverage is 85.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #602      +/-   ##
==========================================
+ Coverage   82.86%   84.53%   +1.66%     
==========================================
  Files           9        9              
  Lines        1675     1513     -162     
==========================================
- Hits         1388     1279     -109     
+ Misses        287      234      -53     
Impacted Files Coverage Δ
src/header.jl 98.16% <75.00%> (+4.93%) ⬆️
src/tables.jl 75.25% <100.00%> (+11.76%) ⬆️
src/utils.jl 81.03% <100.00%> (-3.26%) ⬇️
src/write.jl 87.19% <100.00%> (+4.34%) ⬆️
src/rows.jl 94.56% <0.00%> (-3.27%) ⬇️
src/iteration.jl 18.75% <0.00%> (-0.90%) ⬇️
src/detection.jl 89.05% <0.00%> (-0.13%) ⬇️
src/file.jl 85.44% <0.00%> (+0.45%) ⬆️
... and 2 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 bc9859d...80c4f89. Read the comment docs.

@nalimilan nalimilan merged commit 18f8e51 into master May 6, 2020
@nalimilan nalimilan deleted the nl/CategoricalArrays branch May 6, 2020 07:34
@nalimilan
Copy link
Member Author

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