-
Notifications
You must be signed in to change notification settings - Fork 16
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
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
There was a problem hiding this 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.
@Frank-III that trick with the Now we just need to add an entry for the new transform in the docs, right after the entry for Functional. |
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. :) |
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. |
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 forVector
as well. (I guess to allow changinglevels
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 thecategorical
function still work but make no sense.