Skip to content
This repository has been archived by the owner on May 4, 2019. It is now read-only.

Allow replacing with results of a function #176

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

paulvictor
Copy link

Create another type which can replace with results of evaluating a function, optionally providing the data array itself as an argument. Use case: Replace with samples drawn from the distribution of non NA values.

Use case: Replace with samples drawn from the distribution of non NA values
@nalimilan
Copy link
Member

Thanks, but I'm not sure a dedicated function and type are really needed here. I find an explicit loop clearer, and almost as short as your version:

[isna(dv, i) ? mean(dropna(dv)) : dv[i] for i in eachindex(dv)]

or the in-place version

for i in eachindex(dv)
    if isna(dv, i) dv[i] = m end
end
``

@paulvictor
Copy link
Author

True, but that's the case with each_replacena too, here the example given seems simple, but given that a function can be used opens up a number of use cases, like replacing NA's with values drawn from a distribution described by the non-NA values(which need not be the same for each invocation), here again, this can be replaced by the transformation as you described, but I find this style better, since you make it more declarative(by passing the function(the what)) than imperative(generating and assigning the values(the how)). On a note aside, there are tests failing on travis and the coverage has been reported to be down, but no test errors occur when I build locally, is there anything I'm missing? Attaching a screenshot of how the tests ran, please give your feedback on the same.

screenshot 2015-12-19 22 25 26

@nalimilan
Copy link
Member

Others should tell whether they find it useful before you do more work on it, but I think you should at least merge each_replacenawithfunctionresult into each_replacena. You could simply add a method for Callable arguments.

I'm not sure why the tests are failing at the moment.

@paulvictor
Copy link
Author

@nalimilan , Can you check paulvictor@365f1a3, if that's what you meant? I can provide a squashed commit later if the code makes sense.

@nalimilan
Copy link
Member

Yeah, something like that, but as I said I'll let other comment.

@paulvictor
Copy link
Author

Thanks @nalimilan , will wait too...

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants