Skip to content

Comments

Subset bism#248

Merged
nullsatz merged 10 commits intomasterfrom
subset-bism
Jun 13, 2025
Merged

Subset bism#248
nullsatz merged 10 commits intomasterfrom
subset-bism

Conversation

@nullsatz
Copy link
Collaborator

Added a specific subset method for BlockedInfinitySparseMatrix that does "the right thing" for the groups member. Updated summary.BlockedInfinitySparseMatrix in a minimal way to prevent infinite recursion on summary.

@benthestatistician
Copy link
Collaborator

Good stuff Josh! Does anybody know of an S4 analogue to NextMethod()? Something like that seems to be what's called for at R/InfinitySparseMatrix.R#L1121 --

subIsm <- callGeneric(as(x, "InfinitySparseMatrix"), subset, select)

-- to harden the code and make unnecessary e4b8f96's workaround at R/summary.ism.R#L105. @josherrickson @markmfredrickson @adamrauh

But I'm not sure such a thing exists. Barring an immediate answer from someone, I'm for merging this solution into the master branch and bumping the minor version.

@benthestatistician
Copy link
Collaborator

I don't think we previously had tests that subsetting preserved the BISM class. Could you add one of those @nullsatz , and while you're at it check that the presence of names for the @groups vector is preserved?

@nullsatz
Copy link
Collaborator Author

I've added a pretty simpleminded test here

Please feel free to improve the wizardry.

I also tried to fiddle around with setMethod and callNextMethod for our subsetting methods. I just got weird errors about the subset generic and signatures so I gave up. Maybe someone with more S4 experience can take a look.

@benthestatistician
Copy link
Collaborator

These tests look good to me! Also, I don't see any need for more methods-fiddling -- if somebody thinks of a nicer way to finesse the line I called out a few comments up on this thread, they can always contribute it later.

Were you hoping for Mark's and my reviews both, Josh? If you were just looking for one or the other, pls go ahead and merge. (After bumping the version number from 0.10.8.9001 to 0.10.8.9002, which @kkbrum can then use to check to make sure the optmatch she uses has this particular bug fixed.)

@nullsatz nullsatz merged commit bfc8825 into master Jun 13, 2025
6 checks passed
@benthestatistician benthestatistician deleted the subset-bism branch December 3, 2025 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants