-
Notifications
You must be signed in to change notification settings - Fork 1
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
Replace mapslices with selectdim #45
Conversation
Codecov Report
@@ Coverage Diff @@
## main #45 +/- ##
==========================================
- Coverage 94.84% 94.79% -0.06%
==========================================
Files 9 9
Lines 97 96 -1
==========================================
- Hits 92 91 -1
Misses 5 5
Continue to review full report at Codecov.
|
@@ -35,8 +35,8 @@ | |||
@test FeatureTransforms.apply(x, ohe; inds=2:4) == [0 0 1; 0 1 0; 0 0 1] | |||
@test FeatureTransforms.apply(x, ohe; dims=:) == expected | |||
|
|||
@test_throws DimensionMismatch FeatureTransforms.apply(x, ohe; dims=1) |
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.
dims
no longer takes slices in apply
so it makes no difference to how OneHotEncoding
works unless used with inds
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.
Looks good to me. Are we leaving LinearCombination
for a follow-up?
yep, I'm working on it now |
This PR replaces the call to
mapslices
with a call toselectdim
instead.We change this now because we no longer need to take slices in the parent
apply
method(s) (#42).So we can replace
mapslices
withselectdim
instead, which is simpler and much more performant (see below), yet keeps the desireddims
convention.background
This is a follow up to #42, which removed the need for looping over the slices in
apply
, and #25, which was aimed at solving ourdims
convention #18.We originally liked
mapslices
because we wanted to adopt itsdims
convention, which was more intuitive to our users.However, it has some considerable downsides that make it a poor choice in the long-term, namely:
eachslice
SlicedArrays
and abstracting thedims
argument.eachslice
view
Equivalent of mapslices with views? JuliaLang/julia#29146Its one technical advantage was allowing multiple
dims
, but this isn't a necessary feature right now.Comparing to previous benchmarks this significantly reduces allocations (x100) for element-wise transforms (although
MeanStdScaling
is a bit of an unfair comparison since it has been simplified in the meantime). It might be worth looking to make the rest more efficient also.