markmfredrickson / optmatch

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

inconsistent `exceedances` output #202

Closed josherrickson closed 3 years ago

josherrickson commented 3 years ago

In Optmatch.R#L116-L118, we store the exceedances attribute.

Depending on the scenario, the results are inconsistent. If we have subproblems, it appears that all subproblems working yields a vector, whereas with failing subproblems, it yields a list (because an sapply can't collapse to a vector when there's a NULL).

Which do we want?


> attr(fullmatch(pr ~ cost, data = nuclearplants,
+                within = exactMatch(pr ~ pt, data = nuclearplants)), "exceedances")
          0           1 
0.015019117 0.002911929 

> attr(fullmatch(pr ~ cost, min = 10, data = nuclearplants,
+                within = exactMatch(pr ~ pt, data = nuclearplants)), "exceedances")
$`0`
[1] 0.1008304

$`1`
NULL

This is "no action" until issue #203 is addressed. It's possible that addressing #203 fixes the issue by removing the NULL. If so, the action here would be to replace sapply with vapply(..., numeric(1)) to ensure we don't get this issue again (or some other check for NULL).

josherrickson commented 3 years ago

After addressing #203, this issue remains. This is due to a shortcutting when min/max are certain types of infeasible:subDivStrat.R#L55

1) Do we want to remove this shortcut, so we still get the exceedances? Or is there another way to compute it that I'm not seeing? 2) Assuming the answer to 1. is "No", any objections to replacing NULL with NA so that we can consistently return a vector? I think an update to the documentation explaining when the exceedances are not defied is warranted too.

benthestatistician commented 3 years ago

2. No objections to replacing those NULL with NA. (Besides the usual about limiting collateral damage. Which our tests would probably alert us to, if present.) 1. FYI, the exceedances as currently calculated aren't necessarily well motivated. I hope to replace them with something better as a part of the node prices work. (Something to bear in mind as you allocate time to e.g. related docs updates.)

josherrickson commented 3 years ago

I changed from NULL to NA. Per your suggestion, I did not touch the documentation.