subset.BISM

[benthestatistician]

We need a subset.BlockedInfinitySparseMatrix method, paralleling the current subset.InfinitySparseMatrix(). (Although subset.ISM() applies to BISMs, it returns an ISM. We need to be able to subset BISMs without loss of the BISM class and blocking info.)

[nullsatz]

Doing this work in branch subset-bism

[benthestatistician]

As things stand(InfinitySparseMatrix.R#L23-L41), the @rownames and @colnames slots of an ISM are optional (type "OptionalCharacter" not character). I suspect they'll have to be made mandatory, at least for BISMs, in order to for us to be able to say which entry of the BISM's @groups characterizes which ISM row/column. I suspect it will also have to be made mandatory not optional that the BISM's @groups carry names, names that are to be found within its @rownames or its @colnames. If so, these new expectations can be enforced in new tests.

Question: Given that makeInfinitySparseMatrix() auto-generates row and column names even if not given, should we consider requiring that ISMs as well as BISMs carry rownames and colnames? @markmfredrickson @josherrickson

[benthestatistician]

@nullsatz, an fyi: @markmfredrickson and I got into some related issues in working through #169 a few years ago. There may be some relevant stuff over there & in associated commits.

I'll also note that in a few other contexts it has seemed that it would be helpful for the groups slot to be a data frame of factor variables, rather than a (single) factor variable: while working on #161, for example (cf "These classes should have ... BISM definition"). So that if the user requested an exactMatch() on say gender and agecategory, The resulting BISM's @groups would store those two variables separately, not interaction(gender, agecategory). If do find ourselves modifying this part of the BISM spec, let's assess the added marginal cost of adding this functionality also.

[nullsatz]

In the branch subset-bism, I updated the bism definition to include rowGroups and colGroups instead of just groups. I also updated most of the methods in the obvious way. The subset method now uses callNextMethod. This work is all in R/InfinitySparseMatrix.R.

These are the bism methods that I didn't touch. I'm not quite sure what they do yet. I'll take a closer look at them later today.

subdim.BlockedInfinitySparseMatrix
num_eligible_matches.BlockedInfinitySparseMatrix
dbind

None of the above is tested since there are some methods that still need help.

[benthestatistician]

1. Nice!
2. Thinking about this over the weekend, it struck me that if we were to go forward with the replacement of the @groups slot with @rowgroups and @colgroups, it would help some users to do this within a new defined class, say StratifiedInfinitySparseMatrix, leaving BlockedInfinitySparseMatrix as-is, along with its methods.
3. We'd get a pretty good test suite for the new SISM class by just copying and slightly modifying existing BISM tests. Our next release could then depracate BISM rather than removing it.
4. How about "@rowgroups" and "@colgroups", as opposed to "@rowGroups" and "@colGroups"?
5. Roles of remaining methods, in brief: - subdim() returns distances matrix dimension, c( (# treatments), (# controls) ), by exact matching subproblem
   • num_eligible_matches() tells you the number of treatment-control combinations that the ISM marks as an allowable match (assigns finite distance to)
   • dbind() bind together distinct distances, either DenseMatrixes or ISMs or BISMs (that are taken each to describe matching possibilities among distinct, nonoverlapping collections of units), into a a unified BISM reflecting both the distances and the grouping structure.

[nullsatz]

2. I've copied and renamed the bism stuff in the new file sism.R.
3. I'll create some new tests via copy / paste in future.
4. Done
5. I'll take a look at this next probably.

I think I fixed subsetting the bism's group member here but it needs to be tested.

[nullsatz]

I fixed the issues that you noted in your pull request and separated things into two branches, subset-bism and sism

The only errors in
subset-bism
that I'm seeing now are

Error (test.fullmatch.R:665:1): Hints accepted
<CStackOverflowError/stackOverflowError/error/condition>
Error: C stack usage  7956960 is too close to the limit

Error (test.summary.ism.R:55:1): summary for BlockedISM
<CStackOverflowError/stackOverflowError/error/condition>
Error: C stack usage  7954000 is too close to the limit

Error (test.summary.ism.R:133:1): distanceSummary suppresses distance
<CStackOverflowError/stackOverflowError/error/condition>
Error: C stack usage  7954000 is too close to the limit

I'm not sure why it's doing this.

I have not yet updated the testthat tests.

[benthestatistician]

Thanks for the helpful separation of the 2 dev streams, JB. This way, once those C stack usage errors are sorted out and we've got subset-bism merged into master, we can bump the minor version and invite @kkbrum to re-install optmatch from GitHub with a min requirement of that new version. Meanwhile work could continue on the sism branch.

[nullsatz]

I went line by line commenting and uncommenting lines in subset.BlockedInfinitySparseMatrix. Calling callGeneric and subset.InfinitySparseMatrix don't appear to cause any problems inside subset.BlockedInfinitySparseMatrix.

The C stack errors only appear after I return a BlockedInfinitySparseMatrix.

If I return an InfinitySparseMatrix then there is no C stack error even when I call the machinery for subsetting the groups member.

My next step will be to use the debugger with the failing testthat tests.

[nullsatz]

I tracked the problem down to summary.

This function calls subset expecting an ISM back, and then does summary on that object without qualification.

So as a quick fix, I changed the subset to subset.InfinitySparseMatrix to make sure we didn't get infinite recursion on summary.BlockedInfinitySparseMatrix.

I think that summary for BlockedInfinitySparseMatrix could be simplified now that we have a subset with more machinery inside.

I think subset.BlockedInfinitySparseMatrix is done.

[nullsatz]

I'm going to make a new issue for Stratified ISMs and close this issue since subsetting for BISMs is done and merged.

[benthestatistician]

Tests in test.exactMatch.R should be filled out just a tad to so as to enforce for BISMs created by exactMatch() the expectations mentioned in my June 4 comment above. Our tests of subset.BISM() might at the same time be reviewed to see that they're checking these expectations, although on a quick review they seemed like they probably already have what they need.