-
Notifications
You must be signed in to change notification settings - Fork 3
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
Filter Operation on DataFrame #12
Conversation
source/magpie/dataframe.d
Outdated
foreach(a, b; lockstep(indx.row.index, indx.row.codes)) | ||
{ | ||
ret.indx.row.index ~= a; | ||
ret.indx.row.codes ~= dropper(positions, b); |
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.
in general try to avoid doing allocations in a loop, in particular this can be refactored to
ret.indx.row.index = indx.row.index.dup;
ret.indx.row.codes = indx.row.codes.map!((e) => dropper(positions, e)).array.
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.
Sorry about that 😅
Addressed in 3e3d0bb
Returns: | ||
Filtered DataFrame | ||
+/ | ||
auto filter(alias filterFunc)() @property |
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.
is there any particular reason why you can't use https://dlang.org/phobos/std_algorithm_iteration.html#.filter ?
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.
It was mainly due to difference in data type of Homogeneous and Heterogeneous DataFrame.
Filter the rows of a dataframe based on the values returned by the alias function
* Fixed imports of aggregate * Added filter documentation
The comparison with
homogeneous
branch is intentional to avoid unnecessarily long diffs. Once #11 is merged, I'll change the PR to point to master.This was necessary as the homogeneous optimisation isn't in master yet.
Cc/ @thewilsonator