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

SPEEDUP: weighted*() functions allocate 'w' to ones if missing #22

Closed
HenrikBengtsson opened this issue Apr 6, 2015 · 6 comments
Closed

Comments

@HenrikBengtsson
Copy link
Owner

The weightedMad() and weightedVar() used

  # Argument 'w':
  if (missing(w)) {
    # By default use weights that are one.
    w <- rep(1, times=n);
  } else if (length(w) != n) {
    stop("The number of elements in arguments 'w' and 'x' does not match: ", length(w), " != ", n);
  }

and weightedMedian() lets the argument default as:

weightedMedian <- function(x, w=rep(1, times=length(x)),  ...)

This is (a) inconsistent, but also (b) inefficient. It's better to avoid allocate an n-vector of ones and instead call the non-weighted estimator.

PS. The weightedMean() function requires 'w' to be given.

@HenrikBengtsson
Copy link
Owner Author

DECISION:

  • Don't use missing() anywhere, which means weightedMad() and weightedVar() should be updated to get default values for argument w.
  • Add default value for w also to weightedMean().

@HenrikBengtsson
Copy link
Owner Author

Actually, let's go with w=NULL as the default everywhere weights are used, cf. Issue #25.

@mmaechler
Copy link

of cause it's your decision..... but not using missing(.) anymore means not to use powerful and transparent R-like semantics... just because other languages don't provide it ?

@HenrikBengtsson
Copy link
Owner Author

My main concern with missing() has always been that one cannot specify that an argument should be "missing" programatically. There are a few places in the matrixStats where we do nested calls and where this would be useful. It might also be useful to developers using matrixStats.

For example, using (say) NULL as a place holder for the default, I can do:

if (!useWeights) w <- NULL
foo(..., w=w)

This is also backward compatible with how missing() is used (modulo that we need to reserve NULL for this special case).

Without such a place holder and instead using missing(), I need to do:

if (useWeights) {
  foo(..., w=w)
} else {
  foo(...)
  ## or explicitly foo(..., w=)
}

With several arguments, I have to cover all permutation, or use:

args <- list(..., w=w)
if (!useWeights) args$w <- NULL
do.call(foo, args)

... or is there another way?

@gmbecker
Copy link

Hi. I commented to this effect here: https://github.com/HenrikBengtsson/Wishlist-for-R/wiki

But you can specify it programmatically. Just do

foo(xmissing=,) for the xmissing parameter to be missing. If xmissing were also in ... that was passed to foo, you'd get an argument specified twice error, but that will be true of any such method.

@HenrikBengtsson
Copy link
Owner Author

Thanks @gmbecker; first I though "interesting; how did I not know about this one", but then I realized I do (see my Sept 14 comment above) - so, as usual, bad memory on my end.

Your comment made me help clarify what I'm exactly looking for; a way to explicitly pass a value such that it is interpreted as missing, e.g.

if (miss) w <- missing() else w <- 1
foo(w=w)

Currently, the only way this can be done is as:

if (miss)
  foo(w=)
else
  foo(w=1)

This becomes unwieldy when there are many arguments that may need to be passed as missing. I'm looking for a solution where I call foo() in only one place. (My only solution I know is to use do.call() as in Sept 14 comment).

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

No branches or pull requests

3 participants