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

Fix constructors and conversion between categorical arrays #63

Merged
merged 2 commits into from
Mar 20, 2017

Conversation

nalimilan
Copy link
Member

Previously only the conversions between CategoricalArrays and between
NullableCategoricalArrays were using specific methods. This means conversion
between these two types did not preserve levels, ordering or reference type.
Use broader signatures to cover this, and throw an error if nulls are found
when converting to CategoricalArray. Make tests more systematic to cover
all possible combinations; remove old tests which are now redundant.

Also drop the guaranty that constructors always return a copy. This is
inconsistent with e.g. Array(), would have made the code more complex,
and would trap users who thought the behavior was the same as for convert().
By the way, this an inconsistency in the default value for the 'ordered'
argument to constructors, and fix docstrings: the default is now always
to preserve the corresponding property of the input.

Fixes #62.

@codecov-io
Copy link

codecov-io commented Mar 19, 2017

Codecov Report

Merging #63 into master will increase coverage by 0.67%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #63      +/-   ##
==========================================
+ Coverage    91.3%   91.97%   +0.67%     
==========================================
  Files           9        9              
  Lines         506      511       +5     
==========================================
+ Hits          462      470       +8     
+ Misses         44       41       -3
Impacted Files Coverage Δ
src/pool.jl 96.84% <100%> (ø) ⬆️
src/array.jl 94.04% <100%> (+1.43%) ⬆️

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 b88769b...ec3aa80. Read the comment docs.

@coveralls
Copy link

coveralls commented Mar 19, 2017

Coverage Status

Coverage increased (+0.7%) to 91.977% when pulling 0c57e8c on nl/constructors into b88769b on master.

@cjprybol
Copy link
Contributor

Looks great Milan! Thanks for your work here. I've also learned several things, like how your commit messages auto-populate the fields of the pull request. I had noticed that the first line of commit messages will auto populate the title of a PR, but I had no idea the extended body of a commit would auto populate the first message of a PR. That's a big incentive for putting more effort into commit messages.

Is the plan to merge this and tag a new version, and then I'll add modify JuliaData/DataTables.jl#30 to remove the levels! reset I added to unstack and bump the version number of CategoricalArrays to the newly tagged version?

@coveralls
Copy link

coveralls commented Mar 20, 2017

Coverage Status

Coverage increased (+0.7%) to 91.977% when pulling ebbf66b on nl/constructors into b88769b on master.

Previously only the conversions between CategoricalArrays and between
NullableCategoricalArrays were using specific methods. This means conversion
between these two types did not preserve levels, ordering or reference type.
Use broader signatures to cover this, and throw an error if nulls are found
when converting to CategoricalArray. Make tests more systematic to cover
all possible combinations; remove old tests which are now redundant.

Also drop the guaranty that constructors always return a copy. This is
inconsistent with e.g. Array(), would have made the code more complex,
and would trap users who thought the behavior was the same as for convert().
By the way, this an inconsistency in the default value for the 'ordered'
argument to constructors, and fix docstrings: the default is now always
to preserve the corresponding property of the input.
This was not visible before since the convert() method for arrays explicitly
called ordered!() even if it should not have been necessary.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.7%) to 91.977% when pulling ec3aa80 on nl/constructors into b88769b on master.

3 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.7%) to 91.977% when pulling ec3aa80 on nl/constructors into b88769b on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.7%) to 91.977% when pulling ec3aa80 on nl/constructors into b88769b on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.7%) to 91.977% when pulling ec3aa80 on nl/constructors into b88769b on master.

@nalimilan nalimilan merged commit 8434ae6 into master Mar 20, 2017
@nalimilan nalimilan deleted the nl/constructors branch March 20, 2017 13:21
@nalimilan
Copy link
Member Author

Looks great Milan! Thanks for your work here. I've also learned several things, like how your commit messages auto-populate the fields of the pull request. I had noticed that the first line of commit messages will auto populate the title of a PR, but I had no idea the extended body of a commit would auto populate the first message of a PR. That's a big incentive for putting more effort into commit messages.

Actually, it doesn't when there are several commits. But copy-pasting the commit message is pretty effective too. :-)

Is the plan to merge this and tag a new version, and then I'll add modify JuliaData/DataTables.jl#30 to remove the levels! reset I added to unstack and bump the version number of CategoricalArrays to the newly tagged version?

Yes, see JuliaLang/METADATA.jl#8417.

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.

NullableCategoricalArrays constructor and levels ordering
4 participants