Stats speed up

[blackshadowshade]

I'm testing out Posit AI to see if it can speed up the chronically slow colouring-in of the stats table.

My prompt:

In calcButtonMatchupWinStats, I use condformat to colour in the stats table based on win percentage. It works correctly but it is very slow. Can you suggest a faster alternative?

Its substantive responses:

The bottleneck is that condformat is doing a lot of work internally to apply cell-level formatting. Since you've already computed the color index vector (output.colour), you can bypass it entirely and build the HTML table directly with inline styles — which is much faster.

and

The approach: instead of condformat building the table (which involves NSE evaluation, S3 dispatch, and knitr rendering internally), the replacement directly constructs the HTML string using vapply over the rows. The color mapping is a simple vector lookup, and htmltools::htmlEscape handles any special characters in button names safely.

The only dependency added is htmltools, which is almost certainly already installed since it's a transitive dependency of many common packages (knitr, shiny, etc.).

If condformat is no longer used anywhere else after you eventually replace the player stats version too, you can remove it from the box::use() block entirely.

I'm going to take a close look at what it's done here and also test it. When I understand what it's doing and have had a chance to review it, I'll remove the incomplete tag and request review.

[blackshadowshade]

Improvements step 1:

A few things stand out:

dplyr and %>% are now dead code. The pipe was only used in the condformat chains. Both can be removed.

generateHtmlFile doesn't actually write the coloured tables to disk. It calls print(..., file = ...), which is xtable's print method and knows about the file argument. But when passed a plain character string (what the colour branch returns), it dispatches to print.default, which ignores the file argument and just prints to the console. This means button_matchup_stats.html and player_matchup_stats.html are never written when is.colour = TRUE. The fix is to branch on type:

generateHtmlFile <- function(html.table, fname) {
path <- paste0(save_dir(), fname)
if (is.character(html.table)) {
writeLines(html.table, path)
} else {
print(html.table, type = 'html', include.rownames = FALSE, file = path)
}
}

Duplicate HTML table building. The is.colour blocks in the two functions are nearly identical. Extracting a helper (e.g. buildColouredHtmlTable(df, caption)) would eliminate the duplication.

Diagonal assignment is verbose. Both matchup functions have:

n.games.matrix[row(n.games.matrix) == col(n.games.matrix)] <- n.games.matrix[...] / 2

This can just be diag(n.games.matrix) <- diag(n.games.matrix) / 2.

Commented-out block in calcPlayerMatchupWinStats (lines 405–408) looks like dead code that can be deleted.

The generateHtmlFile bug is the most important — worth fixing first since it may mean those two output files have never been generated correctly when is.colour = TRUE. Want me to apply any or all of these?

[blackshadowshade]

I then got Posit AI to remove the is.colour code branches.

After that, the next set of improvements were:

A few things:

Unused columns. Both game.winner.df constructions (button and player versions) include _name columns that are never used — only the ID columns feed into sparseMatrix. These can be dropped:

calcButtonMatchupWinStats - currently:

game.winner.df <- data.frame(
winner_button_id = data.df$alt_button_id[data.df$did_win],
winner_button_name = data.df$button_name[data.df$did_win], # unused
loser_button_id = data.df$alt_button_id[!data.df$did_win],
loser_button_name = data.df$button_name[!data.df$did_win] # unused
)

Duplicated colour computation. The output.colour / colour.map / bg block is identical in both matchup functions. Since buildColouredHtmlTable already assumes a specific 4-column structure (it hardcodes which column gets the background colour), you could absorb the colour computation into it — taking just df and caption — and let it read 'Win %' and '# games played' directly. The call sites then become a single line each.

Minor style issues:

save_dir() has a trailing semicolon on return(dir_path); — valid R but unconventional.
The connectToDatabase comment says "connect via TCP/IP" but the connection actually uses unix.sock. Should probably say "via Unix socket".

[blackshadowshade]

The third set of improvements were:

A few more things worth noting:

Vectorize buildColouredHtmlTable. The vapply loop extracts df[i, ] on each iteration, which allocates a new single-row data frame each time. All four cell types can be built as vectors and combined without a loop:

rows <- paste0(
'',
'', htmltools$htmlEscape(as.character(df[[1]])), '',
'', htmltools$htmlEscape(as.character(df[[2]])), '',
'', df[[3]], '',
'', df[[4]], '',
''
)

diag() for the win percentage matrix diagonal. Both matchup functions still use the verbose form for zeroing out the diagonal — the same pattern we already simplified in the n.games.matrix step:

current:

win.percentage.matrix[row(win.percentage.matrix) == col(win.percentage.matrix)] <- NA

simpler:

diag(win.percentage.matrix) <- NA

pmin() to clamp the log frequency matrix in calcButtonMatchupsPlayed. Three lines become one:

current:

log.freq.matrix.limited <- log.freq.matrix
upper.limit <- 5
log.freq.matrix.limited[log.freq.matrix.limited > upper.limit] <- upper.limit

simpler:

log.freq.matrix.limited <- pmin(log.freq.matrix, 5)

Extract a buildFrequencyMatrix helper. The data.table → sparseMatrix → as.matrix pipeline is duplicated across both matchup functions. It could be pulled into a small helper taking winner_ids, loser_ids, and n.

on.exit() for the database connection. If any query throws an error in runAll, dbDisconnect is never called. Registering it with on.exit immediately after connecting would make the cleanup reliable.

[blackshadowshade]

The final set of improvements:

The code is in good shape. There are a handful of smaller things left:

buildFrequencyMatrix isn't used in calcButtonMatchupsPlayed. That function follows the same data.table → sparseMatrix pipeline manually. If the parameter names were generalized from winner_ids/loser_ids to row_ids/col_ids, the helper could serve both places and games.played.df would disappear entirely.

win.percentage.df.short is an unnecessary copy. It's created solely to rename columns before writing the CSV and JSON. setNames() inline removes the intermediate variable:

write.table(
setNames(win.percentage.df, c('b1', 'b2', 'wp', 'ng')),
file = 'win_percentage_stats.csv',
col.names = c('button_1', 'button_2', 'win_percentage', 'number_of_games'),
...
)

Finding unplayed matchups builds a large intermediate data frame unnecessarily. which(..., arr.ind = TRUE) on n.games.matrix directly gives the zero-entry indices without constructing n.games.df:

zero.idx <- which(n.games.matrix == 0, arr.ind = TRUE)
unplayed.df <- data.frame(
button1 = button.names.df$button_name[zero.idx[, 1]],
button2 = button.names.df$button_name[zero.idx[, 2]]
)

Two stale comments worth cleaning up: the colorRamps note at the top (left over from the original import list) and the queryButtonStats function comment which says "ignoring mirror matches" — the query doesn't actually filter them, it just computes the flag; the filtering happens later in calcSingleButtonStats.

[blackshadowshade]

In total, this improvement and refactoring work used up about 75% of my free trial credits.

[cgolubi1]

Just a placeholder to note that we'll need an AI dev policy before the first time we merge code that was written with AI assistance. I can try to look up some examples, or we can just make up something we can live with.

For Button Men specifically, i'd want to focus on:

• testability --- do we have tests that humans can understand that validate the functionality works and doesn't have unintended consequences?
• attribution - make sure we know what code was written with the assistance of what versions of what tools, so that if at some point in the future of open source there's a whole IP mess, we'll at least know where to look

(I don't use AI tools in my hobby coding for a few reasons, but whatever we agree to, i am of course also happy to do it.)

[blackshadowshade]

Thanks for raising this. This is partly why I thought I'd evaluate AI coding openly on a piece of code that is peripherally associated with our BM project.

If you could link to a policy that already exists and is easy to understand, that would be great. I'd be happy for us to adapt it if necessary, much like our Privacy Policy was adapted.

[blackshadowshade]

I've run the transformed script and it runs within seconds, which is a huge improvement over the 6 hours that the original required.

I've done a side-by-side comparison of the tables and the contents are essentially the same, the only difference is the table formatting and the truncation of decimal places when the value is a round number.

I've done a diff of the .csv and .json files and they are identical.

It looks like the final script does what we need it to do.

Next step is to look at the code changes.

[blackshadowshade]

Makes sense.

[blackshadowshade]

Verified that this is just refactoring out a function.

This is using inputs of the first button, the second button, and the max button ID.

[blackshadowshade]

This is using the refactored function but with inputs of the winner, loser, and max button ID.

[blackshadowshade]

Explicitly applies the halving to the diagonal as intended.

[blackshadowshade]

Colour computation is exactly my code.

[blackshadowshade]

bg is a vector of colour strings

[blackshadowshade]

Build a raw HTML header

[blackshadowshade]

Build raw HTML rows

[blackshadowshade]

Build raw HTML table and include the HTML header and a HTML body containing the rows

[blackshadowshade]

Find unplayed matchups directly from the button matchup frequency matrix

[blackshadowshade]

Directly create the data frame of unplayed matchups pertaining to unplayed matchups

[blackshadowshade]

Explicitly sets the diagonal (mirror matches) to NA to exclude them from win percentage calculation.

[blackshadowshade]

Inline setting of column names

[blackshadowshade]

Inline code

[cgolubi1]

Oh, as in it's just changing the code formatting? Sure. (I wasn't sure what "inline code" meant until i looked here.)

[blackshadowshade]

Yeah, it's just "inlining" multiple preparatory lines of codes into the final line.

[blackshadowshade]

Use the new Html table generator, and delete the non-colour branch of the code.

[blackshadowshade]

Changes in this function are effectively the same sort as in calcButtonMatchupWinStats.

[blackshadowshade]

New branch here deals with the raw HTML table that was created.

[blackshadowshade]

Handle disconnection more carefully.

[blackshadowshade]

I've just noticed that I created the pull request from a branch of buttonmen-dev instead of my own personal repo, so I'm going to close this and recreate a new pull request from my personal repo. I'll link this pull request from it.