Improvements to hinting and subproblem handling

[adamrauh]

Hi all,

This PR contains a number of changes aimed generally at improving hinting and handling subproblem metadata. The major proposed changes are as follows:

1. Improvements to hinting aimed at closing out issue 76. This functionality was in a pretty far-along state when I picked up work on it recently, so most of the recent work here amounted to adding some documentation, examples, and (I think) tests.

2. Explicit tracking of subproblem feasibility that has the immediate implication of (hopefully) closing out issue 178. Practically speaking, changes here ensure that an MCFSolutions object is always returned from pairmatch() and fullmatch(), regardless of feasibility, and regardless of where in the stack the subproblem is found to be infeasible (if it is infeasible). So, we can rely on that subproblems table existing and pull from it accordingly.

I'll flag that I did change a test in the implementation of (2). Substantively, if a problem consists of only one subproblem, the subproblem group now gets listed as "", as opposed to "1" in the subproblems table, as it was previously. The test in question that I changed can be found here. I have no strong doctrinal reason for this and also am not sure if this breaks important expectations, so of course open to changing things around.

3. The ability for users to specify subproblem-level resolution values to pairmatch() and fullmatch() via a new argument, resolution.

4. Some under the hood refactoring to improve the clarity of the lower-level functions that handle solving individual subproblems (e.g. .fullmatch_with_recovery(), fmatch(), solve_reg_fm_prob(), a new function parse_subproblems())

I'll refer to this thread on my fork of the repo for more information/discussion of what's in here. I'll note that I'm requesting into the proj1-nodeprices branch, per @benthestatistician 's request -- I'm not sure what else lives in that branch.

With the caveat mentioned above in (2), all tests are currently passing on my machine. Please let me know how else I can help!

[benthestatistician]

Some initial comments...

subproblems

1. I like parse_subproblems()
2. I like the fact that the new subproblemSuccess() informatively names its output. Could this be made into an expectation, enforced with tests (eg in tests/testthat/test.optmatchS3.R)?
3. Does the PR make a change to how the absence of subproblems is encoded in the nodeprices table, from "1" to ""? If so, make sense, but can the PR modify vignettes/MCFSolutions.Rmd to note this while removing the "(temporarily)" qualifier from tests/testthat/test.optmatchS3.R#L212?

inline comments

1. A number of inline comments are on a single extended line. It would be well to end these at 80 characters, or extend them over multiple lines in order to keep each line to 80 characters. Instances:

• R/solve_reg_fm_prob.R#L27
• R/fmatch.R#398

2. There are also a number of inline comments that express reservations about what's being done, sometimes also stating a wishlist of improvements. (This category of comment has overlap with the prior category.) Could the regrets be replaced with factual, non-judgmental flaggings of a hazard or anomaly, or simply removed? And those editorial comments that pertain to what we might like to do in the future be moved to comments in this PR thread? And/or to self-standing issues, as appropriate. Instances:

• R/fmatch.R#L181
• R/solve_reg_fm_prob.R#L99
• R/solve_reg_fm_prob.R#134
• tests/testthat/test.optmatchS3.R#212

3. Could the "As of in the nodeprices branch..." qualifiers be removed from the MCFSolutions.Rmd vignette?

[benthestatistician]

Nice stuff here, collecting all these calculations under the "utils" banner and giving them informative names. Some of this would have been a little painful, but it's a nice win for readibility/maintainability and I appreciate it.