markmfredrickson / optmatch

Functions for optimal matching in R
https://markmfredrickson.github.io/optmatch
Other
47 stars 14 forks source link

simplify `match_on.function()`'s expectations of its 1st argument #205

Open benthestatistician opened 3 years ago

benthestatistician commented 3 years ago

match_on.function() expects its first argument to be a function with 3 arguments, index, data and z. But in many cases, including each of my uses of it as well as the example discussed inline in the documentation, only the first two are used. This adds a disorienting element to our explanation:

...This may sound complicated, but is simple to use. For example, a function that returned the absolute difference between two units using a vector of data would be f <- function(index, data, z) { abs(data[index[,1]] - data[index[,2]]) }.

  1. I suspect that we could simply do without the z= argument.
  2. Alternatively, I'd think we could accommodate functions passed to match_on() that either do or do not take a z= argument, provided that we insist they take index and data arguments. In particular, before this point in makedist.R
    dists <- distancefn(cbind(treatmentids, controlids), data, z)

    we check whether any(names(formals(distancefn))=="z") and if not then instead do

    dists <- distancefn(cbind(treatmentids, controlids), data)

Either way this is an API change appropriate to a major version bump (#134) .

benthestatistician commented 3 years ago

Would you see any reason to hesitate with either of the options proposed in this issue, (1) or (2), if they didn't break the current tests? @markmfredrickson @josherrickson

josherrickson commented 3 years ago

Anything that simplifies match_on.function gets my support. What about also supporting a version that just takes in a pair of points and returns a distance between them; handling the index internally? I feel like that would live up to the promise of "just define any distance metric" better than having users worry about vectorization of functions.

markmfredrickson commented 3 years ago

No argument about removing z. I'm sure I had a reason for that at some point, but looking at the test file doesn't include any examples.