-
Notifications
You must be signed in to change notification settings - Fork 14
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
subproblemSuccess() to pull its answers from MCFSolutions@subproblems table #178
Comments
As things currently stand [proj1-nodeprices ce38fbc], there are a few conditions where we don't record subproblem info, e.g. solve_reg_fm_prob.R#L87-L94: if (floor(min.cpt) > ceiling(max.cpt) | #inconsistent max/min
ceiling(1/min.cpt) < floor(1/max.cpt) | #controls per treatment
!rfeas | !cfeas # either no controls or no treatments
)
{
ans <- rep(NA_integer_, nrows + ncols)
names(ans) <- c(rownames, colnames)
return(list(cells=ans, err=0, MCFSolution=NULL)) We'll have to address this in order to address the current issue. It seems like it could come in handly also to have captured partial info for the nodes table, if this can be done without ruining semantics of columns we won't have reliable info for when we haven't located a meaningful subproblem (e.g. |
@adamrauh reports via email that subproblems tables are showing data(nuclearplants)
mm <- match_on(pr ~ cost + t1 + t2, data=nuclearplants)
# not infeasible as given
f <- fullmatch(mm, data=nuclearplants, max.controls=3)
attr(f, "MCFSolutions")@subproblems
Object of class "SubProbInfo"
groups flipped hashed_dist resolution lagrangian_value dual_value feasible exceedance
1 FALSE 623.423420488867 0.001066667 34.2503 34.22637 FALSE -0.003563345 which appears to be a bug. It seems that the " optmatch/R/solve_reg_fm_prob.R Line 130 in 4d9e342
solution==-1L ". This based on my recollection of things, buttressed by how I see fmatch() dealing with infeasibility, e.g. here:Lines 139 to 151 in 4d9e342
|
It appears that something wonky might be happening here re: tracking feasibility. Using the following as a problem that should be infeasible:
generates a warning about infeasibility: There are two attempts to solve from within Line 569 in 4d9e342
and Line 587 in 4d9e342
If I take a look at how the
so it doesn't look like that condition is getting tripped in fmatch: Lines 139 to 151 in 4d9e342
Not sure what the solution is yet, but just documenting for now @benthestatistician |
In this case, the original f.m. problem was infeasible but the two f.m. problems that you call out, producing |
Ah, ok -- thanks. In that case, I think the solution might be just as straightforward as you initially expected. I think perhaps we want With the change implemented here:
as expected |
Nice! Could you also add tests asserting of a known infeasible subproblem that it's recorded as such, and also of a known feasible problem that it's recorded as such? And also of a problem with some feasible and some infeasible subproblems that they are recorded as such? (E.g., start by breaking the nuclearplants example into 2 exact matching classes based on value of As to |
The following two lines of Lines 238 to 240 in 4d9e342
and with the RELAX-IV solver: Lines 256 to 258 in 4d9e342
My conclusion would be that any(temp$solution==1L) should be used as the test of feasibility.
|
... in addition, Lines 319 to 325 in 4d9e342
This might be addressed in tandem with filling out an MCFSolutions object to return with infeasible problems for which the solver winds up being bypassed: Lines 139 to 150 in 4d9e342
Potentially this should be broken out as its own minor issue. |
Just flagging that I've amended an old commit here to reflect this logic. Of course, this doesn't deal with the potential MCFSolutions problem, so just a small patch for now. |
I've documented some relevant updates over here. But, to summarize: Streamlining the UI to always include an explicit tracking of feasibility via the |
Hi @adamrauh, where do things stand on your work addressing this issue and hinting improvements? I'm fuzzy on details from our last convo, but suspect there may been just a few things to wrap up before you'd be in a position to post a PR. If that's so, perhaps the current intercession would provide opportunities to wrap up your work and prepare and post a PR. If it's not so, or if this intercession is no good for that work, I'd much appreciate an update here or elsewhere that's accessible from here. |
..when that table is available. Its current logic can serve as a fallback for cases where it isn't.
The text was updated successfully, but these errors were encountered: