Repository navigation
harmonize r style - #65
Merged
Merged
Conversation
Write the conventions down in contributing.md so the harmonization that follows has something to point at: native pipe, one alphabetized setup chunk, no pkg::fun(), individual packages over library(tidyverse), current dplyr/tidyr idiom, \(x) lambdas, labs(), kebab-case chunk labels, here() for data/ paths, and plain fences for illustrative code. Also note that read.csv() in the CRF appendices is deliberate -- read_csv() reports parsing errors on those raw exports -- so it is not a target. This commit only touches chapters with no executable R, so nothing here needs a re-render: - cat12.qmd: drop the df-print front matter. The chapter has no R chunks, so it was configuring a table printer that never runs. - genomic_imputation.qmd: snake_case and spacing in the two supplementary scripts. air and panache skip plain ```r fences, so these had never been formatted. Also drops a duplicated re-read of the same .fam file. - contributing.md: the df-print example pointed at freesurfer.qmd, which has no front matter; bids-qc-joining.qmd is the actual example. Declare r-here and r-tibble in pixi.toml. Both are used across the kits but were only present transitively, the same gap 5f1722f closed for the others. No package versions change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Code: - library(tidyverse) -> dplyr, ggplot2, here, readr, the four packages the kit actually uses. - read_csv() paths go through here() instead of being relative to the render directory. - the one %>% becomes |>. Prose: - the chapter had "## Considerations While Working on the Project" twice, with near-identical paragraphs under "### Methods" and "### Data Generation". Merged into one section; the surviving paragraph drops a "(see below for more details)" that pointed at the duplicate. - "functiona" -> "function", "assessement" -> "assessment". - #| label: peak -> peek. The rendered output also picks up the current text of _snippets/a2cps_citations.qmd. fa43a07 edited that snippet without clearing the frozen output of the chapters that include it, so this chapter had been publishing the pre-edit citation blurb. eddyqc, genetic_variants, mri-derivative-qc, mriqc and raw-mri are still stale the same way and will correct themselves when they are re-rendered. Rendered with LANG=en_US.UTF-8. Without it R falls back to the C locale and pillar prints ASCII "~" where the committed output has "…", which shows up as a spurious diff in every chapter that glimpse()es a frame. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
These 21 chapters already used |>, so this is the rest of the rules. Setup chunks: one per chapter, immediately after the H1, alphabetized, holding only packages the chapter actually calls. Moved out from mid-document (freesurfer, fslanat, eddyqc, fcn) and from above the H1 (neural-pain-signatures). raw-mri's "vars" chunk keeps its job but no longer doubles as the library block, and its .vars is renamed to variables so there is no hidden top-level object. Dropped six library() calls nothing used: tidyr in brainager, dplyr in dwi_biomarker1, ggdist in postgift, and the lone readr in fmriprep, qsiprep and reconstruction -- those three contain no R at all, so their setup chunk and _freeze are gone entirely. Removed 47 pkg::fun() prefixes and loaded the packages instead, adding setup chunks to ids, mri-idps and nda-mri, which had none. Package references in *prose* keep their prefix: "[`fs::dir_ls`]" tells a reader where the function comes from, which is the point of the rule. Also: six ~ .x lambdas become \(x); xlab()/ylab() become labs(); na.omit() becomes drop_na(); summarize() becomes summarise(); the dangling comma in eddyqc's map() is gone; fcn's anti_join() states its by = join_by(sub) instead of emitting a join message onto the page; colour becomes color in mri-derivative-qc. Two figures were publishing with no caption at all: brainager used "#| caption:" and mriqc used "#| fig-caption:", neither of which is a Quarto cell option, so both were silently ignored. Both are now fig-cap and the captions render. The seven executable cells that only display a command or a JSON payload are plain fences now, per contributing.md -- shortcodes do not expand inside executable cells. reconstruction had a stray "#| eval: false" sitting inside a *plain* fence, where it rendered as a literal line of code. fslanat's two separate() calls become separate_wider_delim(). They need too_many = "drop": names like "Left-Thalamus-Proper" and "Left-Accumbens-area" have to reduce to their stem for the FSL rows to join the FreeSurfer ones, which separate() was doing via a warning that the project-wide warning: false hid. Verified identical on the real structure names. NOT RE-RENDERED HERE, freeze deliberately left at its previous state: fcn, mri-derivative-qc, postgift, dwi_biomarker1, mriqc. This working copy holds a partial data/ tree, and re-rendering them would republish results computed on it -- postgift's model goes from 1286 participants to 584. Their sources are harmonized but their frozen output is not, so the preview will flag them; re-render on a machine with the full release before merging. The source edits are behaviour-preserving: running the old and new sources against the same data leaves identical objects, with a HEAD-vs-HEAD control to separate the edits from fcn's own row-order non-determinism. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
biospecimen, Funtional and Imaging: 594 %>% become |>.
The only magrittr placeholders in these files were 11 nrow(.) dots, all
one shape:
dict <- dict %>%
slice(seq_len(nrow(.))) %>%
add_row(.after = nrow(.), !!!new_row)
slice(seq_len(nrow(.))) selects every row in order, so it does nothing,
and .after = nrow(.) is add_row()'s default. Both are gone; the chained
case in Funtional still appends in the same order. The remaining dots are
purrr lambda arguments in ~ replace_na(., "") and are untouched -- those
are bound by the lambda, not the pipe.
The pipe sweep itself is proven inert: with the dots handled first, the
before and after sources parse to identical ASTs across 49, 58 and 92
top-level expressions.
library(tidyverse) becomes the packages each file actually calls. forcats
and janitor were loaded by all three and used by none; hablar is only
used by Funtional, for retype().
here::here() becomes here() at 16 sites -- the files used the bare form
for reads and the namespaced form for writes, for no reason. top_n(1, x)
becomes slice_max(x, n = 1) at 12 sites, mutate_all() becomes
across(everything()) at 6, and a trailing TRUE ~ becomes .default = at 8.
Two labels: data-diciontary -> data-dictionary, and the Imaging kit had a
chunk labelled "qst" copied from the QST kit, now imaging-qst.
Verified twice. Running the old and new sources against the same data
leaves every object identical up to row order -- slice_max() sorts where
top_n() preserved input order -- and the rendered output is byte-for-byte
unchanged in all three chapters, so that reordering never reaches a page.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
885 %>% become |>. This file had both magrittr placeholder shapes. The 14 left_join(., y, by = ...) sites are the ones worth care. magrittr suppresses first-argument insertion when a `.` appears as a top-level argument, so `x %>% left_join(., y)` already means `left_join(x, y)` -- verified -- and the fix is to delete the `.,` line. Dropping just the dot instead would leave `left_join(x, ., y)`, which is a hard error under |>, not a silent wrong answer. The other 20 dots are the same no-op slice(seq_len(nrow(.))) and default .after = nrow(.) removed from the other CRF appendices. With the dots gone, the sweep is proven inert: the before and after sources parse to identical ASTs across 166 top-level expressions. library(tidyverse) becomes the seven packages the file uses. here::here() becomes here() at 4 sites, top_n(1, x) becomes slice_max(x, n = 1) at 4, mutate_all() becomes across(everything()) at 4, and a trailing TRUE ~ becomes .default = at 39. The 40 rowwise() calls are LEFT ALONE. They are not the redundant rowSums(across(...)) wrapper they look like from a distance -- they are followed by mean(c(a, b, c)), which is scalar-reducing. Under rowwise() that is a per-row mean; without it, it collapses to one grand mean recycled down the column. On c(1,10)/c(2,20)/c(3,30) that is "2, 20" versus "11, 11". Removing them would silently corrupt every derived QST summary in the file. Verified: old and new sources leave all 140 objects identical up to row order (slice_max sorts where top_n preserved input order), and the rendered output is byte-for-byte unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
395 %>% become |>. This file held 68 of the repo's 113 magrittr placeholders, in two shapes that both translate to pick(): 60x rowSums(!is.na(select(., all_of(v)))) -> pick(all_of(v)) 6x rowSums(!is.na(.[, cols])) -> pick(all_of(cols)) pick() and select(., ...) diverge silently when an earlier argument of the same mutate() creates a column in the selection, so that is checked rather than assumed: all 74 dot-bearing mutate() calls in this file take exactly one argument, which makes the two equivalent. The check is in the harness and should be re-run before anyone merges adjacent mutate() calls here -- psycho_social:1228 (all_koos) and :2147 (ssi_all) select columns built by preceding pipe stages and would break quietly. The `.[, cols]` rewrite is anchored on the ", " so it cannot touch the prose citations `.[@wolfe2016]` and `.[Berkeley personality lab]`, which look like the same pattern but are markdown. The remaining 22 dots are lambda arguments in ~ if_else(is.na(.), m, .) and are left alone. With the dots handled, the sweep is proven inert: identical ASTs across 420 top-level expressions. There is now no %>% anywhere in the repo. library(tidyverse) becomes the eight packages actually used -- GGally, ComplexUpset, gt and janitor were loaded across 4988 lines and never called, and ggplot2 was loaded twice. This was also the only setup chunk in the book without a label. here::here() becomes here() at 31 sites, a trailing TRUE ~ becomes .default = at 47, ifelse() becomes if_else() at 31 (the file already mixed both), applyFilter becomes apply_filter, and the trailing return(filtered_data) is dropped. Labelling the setup chunk shifts every downstream unnamed-chunk-N by one, so 53 figures are renamed. Their content is unchanged: hashing the decoded pixels of every figure before and after gives the same 53 distinct images, with none lost and none gained. Verified: old and new sources leave all 157 objects identical -- exactly, with no sorting needed -- and the rendered output differs only in those figure filenames and in the prose describing apply_filter. The numeric output is byte-identical. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…tive disk
Every write chunk in the CRF and psychosocial appendices builds its path
with here(), and 125 of those paths do not point at a directory that
exists:
37 omit "pre-surgery", e.g. here("data", "blood-draw", ...)
58 say "Reformatted" where the directory is "reformatted"
30 say "Psychosocial" where the directory is "psychosocial"
All of them sit in chunks marked eval: false, so nothing has ever run
them, and macOS is case-insensitive by default, so the casing errors
would work on a developer's laptop and fail on Linux -- which is where
these scripts are meant to run. All 11 distinct write targets now resolve
against the real data/ tree.
The rendered output changes only where the prose repeated the mistake:
four sentences in biospecimen and two in Imaging told the reader to look
in a folder named "Reformatted". Nothing else in any of the five chapters
moves.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
postgift plotted the wrong data. It computes run1_amplitudes_without_red, the prose above the figure says "let's exclude the 'red' scans", and the plot was then handed run1_amplitudes. The published MA plot has been showing the scans the text says are excluded. Source only -- this chapter is in the set awaiting a full-data re-render. postdtifit coerced sub with as.numeric() and then joined on the resulting double; every sibling kit uses as.integer(). Rendered output is unchanged, so the join was working, but the type disagreed with the five other chapters doing the same thing. bedpostx assigned gamma <- 1.8 and never read it, hardcoding 1 / 1.8 in four places instead. Now wired up, and renamed to gamma_correction so it does not shadow base::gamma. Its comment said "take every 10th slice" over a filter that takes every 5th; the prose above the chunk already said every fifth. Numerically identical output. genetic_variants read and wrote through two hardcoded /Users/sethberke paths, which nobody else can run. They are relative now, matching the ./-prefixed style used throughout that script. No personal paths remain anywhere in the repo. genetic_variants also called read_table2(), deprecated in readr 2.0, now read_table(). Its rendered output additionally picks up the current text of _snippets/a2cps_citations.qmd, which fa43a07 edited without clearing the freeze of the chapters that include it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Renames 20 chunk labels that were actively unhelpful, and leaves the
other 465 unlabelled chunks alone. Labelling is not required here --
.panache.toml switches missing-chunk-labels off -- and labelling
everything would rename 142 tracked figures for a change no reader sees.
contributing.md said "label every chunk", which contradicted the repo's
own config; it now describes what to do when you do label one.
Eight labels shadowed a function name: filter and gather (eddyqc),
freesurfer (fslanat), fcn, and session/image/rating/coordinate
(mri-derivative-qc). load, join and plot are conventional chunk names and
are kept. Twelve ran words together: helpercor, helpertimecourse,
mainplot, loadfalff, loadstats, loadapi, showimg, imgmeta, fsltidy,
visitsmapping, brainage2, pairs2.
Three of those labels name a figure, so brainage2-1.png,
pairs2-1.png and mainplot-1.png are renamed. The superseded files are
deleted rather than left behind as orphans.
genetic_variants.qmd had never been formatted. air and panache skip plain
```r fences, and they were also skipping four of this file's ```{r}
chunks -- panache reported the file clean while one line ran to 589
characters. Piping each fence body through air directly fixes it, and the
hooks neither complain nor revert afterwards.
That file also carried 93 identifier occurrences in dot.case or camelCase
(number.geno, matrix.missed, bioDF_Mapping_Plink, problem_IIDs), now
snake_case, and three top-level assignments with = that air rewrote to
<- on its own. Its three executable tibble::tribble() calls are
de-namespaced with library(tibble) added to the setup chunk.
The two supplementary scripts in <details> blocks keep library(tidyverse)
and one jsonlite:: call. They are standalone programs meant to run
outside this project, on files that are not in the release, and two of
their dependencies are not in the environment at all.
Verified: eddyqc, fslanat, postdtifit and ids render byte-identically;
gift, brainager and freesurfer differ only in the renamed figure path;
genetic_variants' 19 executed table rows are unchanged, with the rest of
its diff being the reformatted display fences, which are page content.
ids needed a fix to the comparison rather than to the code -- DT mints a
fresh htmlwidget id on every render, the same way gt does for tables, so
its 8 "changed" lines were noise.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
open_dataset() returns rows in whatever order Arrow's threaded reader delivers them, and the select() here drops ses, task and run -- so the 18132 rows collapse to 5862 distinct (sub, source, target) keys, roughly three rows per key, and head() published whichever one arrived first. Rendering the unmodified chapter twice produced different numbers on the page. Sorting before the select makes the output stable: three consecutive runs now agree, and they agree with the values in the committed freeze. Source only -- this chapter is in the set awaiting a re-render on a machine with the full release. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The earlier reproducibility fix covered the DMN table but missed the difumo one feeding the discriminability analysis, which had the same problem: open_dataset() row order is not stable, and head() published whichever rows arrived first. This chapter's data turned out to be complete after all, so it is no longer waiting on a full-data re-render. Evidence: the six rows the committed freeze displayed (sub 10045, rest1, source 1, targets 10-15) reproduce from the current data to seven decimal places, and every numeric value outside the two head() tables is unchanged. The small participant count in subs_with_all_runs is just the filter(n == 4) doing its job -- keeping only participants with all four functional scans -- not evidence of a partial release. The discriminability statistic itself was never order-sensitive: the six values and their row order are identical with and without the sort, from either source. Only the two displayed tables move, and now they are deterministic. plot-discriminability-1.png changes for a reason unrelated to any of this: rendering HEAD's own unmodified source today produces a different PNG than the committed one, with identical inputs. That is graphics environment drift, not data and not this branch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
geom_jitter() places points at random and nothing set a seed, so this figure had never been reproducible: rendering the same unmodified source twice produces PNGs differing by 0.91 on a 0-1 scale, which is the same magnitude as the difference against the committed freeze. Now geom_point(position = position_jitter(seed = 1)), and two independent renders come out byte-identical. This also settles the chapter: its data is complete, so it is no longer waiting on a full-data re-render. The networks tree holds 1266 subjects x 4 modules = 5064 rows, more coverage than participants.tsv (1019), and the rendered markdown is byte-identical to the committed freeze. The figure churn that had it held back was the unseeded jitter, not a partial release. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
position_jitter(height = 0) still jitters horizontally, and nothing set a seed, so the motion comparison figure changed on every render. Seeded now, and two independent renders come out byte-identical for both figures in the chapter. This chapter's data is complete, so it is no longer waiting on a full-data re-render: nrow(all_bold) publishes 1245400 both in the committed freeze and now, the MRIQC reference database is all 252 parquet files, and the group_bold table is unchanged. It was held back on figure churn that turned out to be the unseeded jitter. The remaining output changes are both intended. The quantile figure gains its caption -- it was written as "#| fig-caption:", which is not a Quarto cell option, so it had been silently dropped and the figure published with no caption at all. And the citation blurb picks up the current text of _snippets/a2cps_citations.qmd, which fa43a07 edited without clearing the freezes that bake it in. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two outputs in this chapter were picked non-deterministically from duckdb reads, whose row order is not stable: slice_head(n = 1, by = n_clicks) -- one arbitrary image per click count distinct(sub, task, run) |> head() -- an arbitrary six failed coregistrations Running the first query three times returned img_id 2784, then 576, then 2784 again. Both are sorted now and three consecutive runs agree. Because the selection was arbitrary rather than wrong, the displayed image changes: the chapter now shows img_id 576 (sub-10040) where it showed 2784 (sub-10168). Both have 8 clicks; 576 is simply the lowest id among them, and it will stay that way from here. This chapter's data is complete, so it is no longer waiting on a full-data re-render: nrow(definite_coregistration_failures) publishes 126 both in the committed freeze and now, and all four qc parquet inputs are present. It was held back because its subject ids moved between renders, which turned out to be this, not a partial release. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The amplitude tree now holds 981 subjects across 1962 parquet files, up from 584 across 1210 when this branch started, and the count is stable rather than mid-sync. All 981 appear in participants.tsv, and every model/run combination agrees on the same 981, so the tree is internally consistent. The mixed model therefore moves: 165480 observations over 921 participants after the red-scan exclusion, where the previous frozen output reported 230160 over 1286. Also carries this branch's earlier fix to plot run1_amplitudes_without_red rather than run1_amplitudes, which is what the surrounding prose has always described, so the MA plot changes for that reason as well as the new data. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
|
The preview for this pull request has been removed. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.