Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make scoringutils paper a package Vignette #796

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from
Draft

Conversation

nikosbosse
Copy link
Contributor

@nikosbosse nikosbosse commented Apr 20, 2024

Description

This PR closes #784.

The PR

  • makes some nonsense changes in .Rbuildignore and .Rinstignore and _pkgdown.yml - please ignore
  • makes a small update to the manuscript to improve a heatmap plot
  • adds a new vignette for the manuscript.
  • adds installing tinytex to the github actions workflow so that the vignette can compile.
  • adds an argument to do vignette compression when building the package.

Including the paper as a vignette is a lot harder than I imagined and as a result of that this is a bit of a terrible PR.

TLDR:

  • we can either go with something like this
  • or not include the paper as a package vignette and simply add it to the pgdown website
  • or someone more clever than I comes up with a better solution.

The broader context is that Vignettes are rendered in the package, as well as on the website via pkgdown. - The package Vignettes can be a pdf and using the existing manuscript works fine. However, pkgdown mostly only accepts HTML files. Rendering the current vignette as an HTML doesn't work well because it uses a lot of LaTeX.

  • pkgdown does have an experimental asis option (see here). However, I was not able to make it work - it simply wouldn't compile my PDF, throwing obscure errors.
  • My next idea was to simply include the PDF in a wrapper vignette. See here: https://stackoverflow.com/questions/19716498/using-a-static-prebuilt-pdf-vignette-in-r-package.
  • However, to do that we need to get the PDF file from somewhere
    • the manuscript.pdf file is currently inst-ignored because it is too large (although the file size could likely be reduced a bit by shrinking the images in the PDF)
    • sourcing the file from the inst directory via system.file() means that the large PDF will exist twice in the package: once copied from inst and once as a vignette.

This is where my terrible approach comes from: download the pdf file from the internet when building the vignette. I assume (hope?) this will work on CRAN because I upload the source file. Not sure about github CI and whether that has access to the internet. In any case, the file will have the link to GitHub so people can access everything.

Potential alternative
What we could do (inspired by the forecast package (site, _pkgdown.yml)) is the following:

  • not add the paper as a vignette at all
  • add a navbar entry to _pgkdown.yml with a link to the paper.

This is basically what the forecast package did. They also managed to include the paper as a PDF package vignette. My theory is this:

  • Their approach works because they only have one vignette.
  • The HTML vignette gets produced when building the pgkdown site (that also works in our case, it just looks terrible)
  • They throw it away (i.e. don't show it on the pgkdown site) by removing the articles section and replacing it by a simple link in their navbar.

[Describe the changes that you made in this pull request.]

Checklist

  • My PR is based on a package issue and I have explicitly linked it.
  • I have included the target issue or issues in the PR title as follows: issue-number: PR title
  • I have tested my changes locally.
  • I have added or updated unit tests where necessary.
  • I have updated the documentation if required.
  • I have built the package locally and run rebuilt docs using roxygen2.
  • My code follows the established coding standards and I have run lintr::lint_package() to check for style issues introduced by my changes.
  • I have added a news item linked to this PR.
  • I have reviewed CI checks for this PR and addressed them as far as I am able.

@nikosbosse nikosbosse mentioned this pull request Apr 20, 2024
9 tasks
Copy link

codecov bot commented Apr 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.62%. Comparing base (948f251) to head (72ebb06).
Report is 10 commits behind head on main.

❗ Current head 72ebb06 differs from pull request most recent head 7f9e516. Consider uploading reports for the commit 7f9e516 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #796      +/-   ##
==========================================
+ Coverage   95.60%   95.62%   +0.02%     
==========================================
  Files          21       21              
  Lines        1569     1577       +8     
==========================================
+ Hits         1500     1508       +8     
  Misses         69       69              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@nikosbosse
Copy link
Contributor Author

nikosbosse commented Apr 20, 2024

image

Oh well this fails on CI and I don't know why... It works locally on my machine... Maybe something to do with pdfpages...

EDIT: the failure comes from the fact that all other vignettes are compiled to HTML and github actions don't install tinytex by default. I updated the github actions yaml.

Next issue:

checking sizes of PDF files under ‘inst/doc’ ... WARNING
    ‘gs+qpdf’ made some significant size reductions:
       compacted ‘scoringutils-paper.pdf’ from 1002Kb to 505Kb
    consider running tools::compactPDF(gs_quality = "ebook") on these files

The vignette file is too big. Folloing the suggestions on SO, I added an additional argument to run compression to the build args. It doesn't really work I'm afraid.

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.

Make current manuscript a package vignette
1 participant