Skip to content

Add vignette precompute function and shift parallel vignette to precomputed - #83

Merged
padpadpadpad merged 1 commit into
padpadpadpad:masterfrom
fwimp:precompute_vignettes
Mar 9, 2026
Merged

Add vignette precompute function and shift parallel vignette to precomputed#83
padpadpadpad merged 1 commit into
padpadpadpad:masterfrom
fwimp:precompute_vignettes

Conversation

@fwimp

@fwimp fwimp commented Mar 3, 2026

Copy link
Copy Markdown
Contributor

This pull request shifts the long-problematic quickfit_parallel vignette over to a precomputed pipeline.

It stops the weirdness of mirai package propagation from causing issues with vignette compilation on CI.

To copy from #81:


  • Vignettes that are to be pre-computed now live in the prerender_vignettes folder at the root of the package.
  • R/rTPC-internal-devtools.R now contains a couple of developer-focused functions to make dev work easier (most importantly the .precompute_vignettes() function). These are not built into the final package.
  • Code chunks that create plots MUST have unique names ACROSS ALL PRERENDERED VIGNETTES
  • Code chunks that create plots SHOULD have fig.cap= set to a reasonable value in the chunk options (otherwise you just get forced to have a caption of "plot of chunk chunk_name")
  • Pre-rendered vignettes will spit out their plots into the vignettes/figure folder under their chunk names.
  • Rendered vignettes are made read-only to hopefully provide a "do you really want to modify the rendered one?" moment.
  • Pre-render vignette sources should only be modified in the prerender_vignettes folder
  • To precompute all vignettes in prerender_vignettes, load rTPC and run .precompute_vignettes("prerender_vignettes", cores = 1). (Remember to do this whenever you have made changes to a source vignette)

R CMD CHECK now passes with no errors.

This approach does, however, trigger a note which is bloody hard to suppress. As such I currently have vignettes/figure ignored in .Rbuildignore.

This seems to suppress the note, but the figure may be missing from installed versions of rTPC (though from my experimentation it all seemed fine).

@fwimp

fwimp commented Mar 4, 2026

Copy link
Copy Markdown
Contributor Author

Out of interest, the failing CI check is just that oldrel-1 uses a version of R prior to 4.5.1, which is a requirement of ggrepl as of 0.9.7 (which was released about a week ago). It should go away once oldrel-1 is updated (I guess by github?)

Most importantly the Windows CI check succeeded!

@padpadpadpad

Copy link
Copy Markdown
Owner

Hi @fwimp. This all looks great. Highly unlikely, but if two code chunks from different files have the same name, will plots get overwritten?

Is it possible to render figures in a subfolder of vignettes/figure, with the folder names by the vignette name. I know right now it is not an issue but might be in the future (theoretically).

@fwimp

fwimp commented Mar 6, 2026

Copy link
Copy Markdown
Contributor Author

Hey @padpadpadpad, Frustratingly that is precisely what will happen and there's basically no easy way around it, at least without developing a good chunk of extra code. Bear in mind though this only happens with pre-baked vignettes.

This is because knitr::knit() does not provide a way to specify where figures should be rendered within the function call itself. It always just renders to ./figure/ within the folder it's in. Even if you solved this by rendering each in their own folder and then shifting them later then you still have to monkeypatch the generated Rmd files to change any figure paths baked in (and hope that your regex was good enough to do it without screwing up other things in the code).

Technically you could do a pre-pass on the Rmd to patch in fig.path= args within each R block that renders graphics, but that is certainly not trivial before runtime.

I think if I were to be doing things like that, this would quickly spiral out into being its own package (bakeR anyone?) which is more than I have time for right now.


The only mitigation I could plausibly do (though even this would be pretty brutal) would be to prepend the vignette name to every chunk name in an Rmd, but honestly that is too much work unless we actively need to fix it.

@padpadpadpad
padpadpadpad merged commit f6fc0ef into padpadpadpad:master Mar 9, 2026
5 of 6 checks passed
@padpadpadpad

Copy link
Copy Markdown
Owner

Agree with you @fwimp. Will try get onto CRAN checks this week and submit a new version there.

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.

2 participants