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

add Levels transform #83

Merged
merged 31 commits into from
May 27, 2022
Merged

add Levels transform #83

merged 31 commits into from
May 27, 2022

Conversation

Frank-III
Copy link
Contributor

The way I currently store caches is to mix functions and vector which is absolutely not optimal, but I am not sure if I should only allow change the levels or order of CategoricalArray or should generalized for Vector as well. (I guess to allow changing levels is fine as we specified the levels to guarantee it is not an infinite column), and for orders when it is set to be true for a float column the categorical function still work but make no sense.

@codecov-commenter
Copy link

codecov-commenter commented May 24, 2022

Codecov Report

Merging #83 (c4f50b3) into master (48b502f) will increase coverage by 0.35%.
The diff coverage is 96.55%.

@@            Coverage Diff             @@
##           master      #83      +/-   ##
==========================================
+ Coverage   92.73%   93.09%   +0.35%     
==========================================
  Files          22       23       +1     
  Lines         537      565      +28     
==========================================
+ Hits          498      526      +28     
  Misses         39       39              
Impacted Files Coverage Δ
src/transforms.jl 97.82% <ø> (ø)
src/transforms/levels.jl 96.55% <96.55%> (ø)
src/transforms/parallel.jl 91.66% <0.00%> (-0.14%) ⬇️
src/transforms/select.jl 93.15% <0.00%> (+1.36%) ⬆️

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 48b502f...c4f50b3. Read the comment docs.

Copy link
Member

@juliohm juliohm left a comment

Choose a reason for hiding this comment

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

This is an awesome PR @Frank-III , thank you for the addition. Left a couple of review comments.

src/TableTransforms.jl Outdated Show resolved Hide resolved
src/TableTransforms.jl Outdated Show resolved Hide resolved
src/transforms.jl Outdated Show resolved Hide resolved
src/transforms/levels.jl Outdated Show resolved Hide resolved
src/transforms/levels.jl Outdated Show resolved Hide resolved
src/transforms/levels.jl Outdated Show resolved Hide resolved
test/transforms.jl Outdated Show resolved Hide resolved
test/transforms.jl Outdated Show resolved Hide resolved
test/transforms.jl Outdated Show resolved Hide resolved
test/transforms.jl Outdated Show resolved Hide resolved
Project.toml Show resolved Hide resolved
src/transforms/levels.jl Outdated Show resolved Hide resolved
src/transforms/levels.jl Outdated Show resolved Hide resolved
src/transforms/levels.jl Outdated Show resolved Hide resolved
src/transforms/levels.jl Outdated Show resolved Hide resolved
src/transforms/levels.jl Outdated Show resolved Hide resolved
test/transforms.jl Outdated Show resolved Hide resolved
@Frank-III Frank-III requested a review from juliohm May 25, 2022 01:46
Project.toml Show resolved Hide resolved
src/transforms/levels.jl Outdated Show resolved Hide resolved
src/transforms/levels.jl Outdated Show resolved Hide resolved
src/transforms/levels.jl Outdated Show resolved Hide resolved
src/transforms/levels.jl Outdated Show resolved Hide resolved
src/transforms/levels.jl Outdated Show resolved Hide resolved
src/transforms/levels.jl Outdated Show resolved Hide resolved
src/transforms/levels.jl Outdated Show resolved Hide resolved
src/transforms/levels.jl Outdated Show resolved Hide resolved
test/Project.toml Outdated Show resolved Hide resolved
src/transforms/levels.jl Outdated Show resolved Hide resolved
@Frank-III Frank-III requested a review from juliohm May 26, 2022 05:06
Project.toml Outdated Show resolved Hide resolved
src/transforms/levels.jl Outdated Show resolved Hide resolved
@juliohm
Copy link
Member

juliohm commented May 26, 2022

@Frank-III that trick with the _categorify function was super neat. You avoided a complicated logic by using the first argument of the function as either a function or a vector of levels 👏🏽

Now we just need to add an entry for the new transform in the docs, right after the entry for Functional.

@Frank-III
Copy link
Contributor Author

Thanks @juliohm, that means a lot to me. It is my first PR and I made a lot of mistakes, I would be more careful with white-space, naming convention and make my code more concise and professional next time. Looking forward to my next PR. :)

@juliohm
Copy link
Member

juliohm commented May 27, 2022

Thank you again @Frank-III for this great addition to the toolkit :) I will go ahead and merge this following our discussions above. We do not want to support mixed Symbol/String types in the same function call.

@juliohm juliohm merged commit 6eb3190 into JuliaML:master May 27, 2022
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.

4 participants