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

ojs chunk option include not working #5994

Closed
am-lh opened this issue Jun 21, 2023 · 6 comments · Fixed by #7799
Closed

ojs chunk option include not working #5994

am-lh opened this issue Jun 21, 2023 · 6 comments · Fixed by #7799
Assignees
Labels
bug Something isn't working ojs-engine Issues with the ojs engine
Milestone

Comments

@am-lh
Copy link

am-lh commented Jun 21, 2023

Bug description

Hi,
I am including a observable plot in a html document. I am used to set in the yaml as:

---
format:
  html:
    code-fold: true
execute:
  eval: true
  echo: true
  message: false
  warning: false
  output: true
  include: false
---

and selectively set the include option to true where I need it.
However, doing so, the ojs chunks does not appear when adding the //|include option, the same working perfectly when yaml include option set to true. I have also tried the {ojs, include=TRUE} like mermaid syntax, and all combination in between them.
Seems that the yaml option overwrite the local one?

Steps to reproduce

```{r}
library(palmerpenguins)
ojs_define(data = penguins)
```

```{ojs}
//| panel: sidebar
//| include: true

viewof bill_length_min = Inputs.range(
  [32, 50], 
  {value: 32, step: 1, label: "Bill length (min):"}
)

viewof islands = Inputs.checkbox(
  ["Torgersen", "Biscoe", "Dream"], 
  { value: ["Torgersen", "Biscoe", "Dream"], 
    label: "Islands:"
  }
)

```

::: {.panel-tabset}

## Graph

```{ojs}
//| include: true
Plot.plot({
  facet: {data: filtered, 
          x: "sex",
          y: "island"},
  color: { legend: true },
  marks: [
    Plot.frame({ strokeOpacity: 0.1 }),
    Plot.dot(filtered, {
      x: "body_mass_g",
      y: "bill_length_mm",
      stroke: "species",
      tip: true
    })
  ]
})

```

## Data
```{ojs}
//| include: true

Inputs.table(filtered, {  columns: [
    "body_mass_g", "flipper_length_mm", "species","bill_length_mm"
  ]})

```
:::

```{ojs}

filtered = transpose(data).filter(function(penguin) {
  return bill_length_min < penguin.bill_length_mm &&
         islands.includes(penguin.island);
})

```

Expected behavior

The ojs chunk should appear in the rendered doc when yaml option is false and chunk option are true

Actual behavior

the ojs chunk does not appear, only the two empty tabsets

Your environment

My settings:

 version  R version 4.3.0 (2023-04-21 ucrt)
 os       Windows 10 x64 (build 19045)
 system   x86_64, mingw32
 ui       RStudio
 language EN
 collate  French_France.utf8
 ctype    French_France.utf8
 tz       Europe/Paris
 date     2023-06-21
 rstudio  2023.06.0+421 Mountain Hydrangea (desktop)
 pandoc   3.1.1 @ C:/Program Files/RStudio/resources/app/bin/quarto/bin/tools/ (via rmarkdown)

quarto : "1.3.361"

Quarto check output

[>] Checking versions of quarto binary dependencies...
      Pandoc version 3.1.1: OK
      Dart Sass version 1.55.0: OK
[>] Checking versions of quarto dependencies......OK
[>] Checking Quarto installation......OK
      Version: 1.3.361
      Path: C:\Users\lehuen201\AppData\Local\Programs\Quarto\bin
      CodePage: 1252

[>] Checking basic markdown render....OK

[>] Checking Python 3 installation....OK
      Version: 3.11.4
      Path: C:/Python311/python.exe
      Jupyter: (None)

      Jupyter is not available in this Python installation.
      Install with py -m pip install jupyter

[>] Checking R installation...........OK
      Version: 4.3.0
      Path: C:/Users/lehuen201/AppData/Local/Programs/R/R-4.3.0
      LibPaths:
        - C:/Users/lehuen201/AppData/Local/Programs/R/R-4.3.0/library
      knitr: 1.43
      rmarkdown: 2.22

[>] Checking Knitr engine render......OK
@am-lh am-lh added the bug Something isn't working label Jun 21, 2023
@am-lh
Copy link
Author

am-lh commented Jun 21, 2023

In addition, this problem seems to mess with a table containing rmarkdown equation: with yaml include: false, I have the table ok but not the observable plot, with include: true, observable is ok, but the equation is not parsed.

```{r}
#| label: tbl-eq_cases
#| results: asis
#| include: true
#| tbl-cap: "an equation table"

library(kableExtra)
my_equations<-data.frame(
  num=c("","1","2"),
  text=c(
    "**Equation 3 Cases (q)**",
"$$
Q_{fluff}\\left(t_0\\right)=Q_{inf}.\\left(1-e^{-a_{bio}.BioFact.t}\\right)+\\varepsilon
$$",
"$$
Q_{fluff}\\left(t_0\\right)=\\left(a_{QBio}.BioFact+b_{QBio}\\right).\\ \\left(a_{QSed}.SedFact+b_{QSed}\\right)
$$"))

tab <- kable(my_equations,  
             col.names = NULL, format = "markdown" #,
           ) %>%
    kable_styling(bootstrap_options = c("striped", "condensed"),
                  full_width = F) ; tab
```

image

@cscheid cscheid self-assigned this Jun 21, 2023
@cscheid cscheid added this to the v1.4 milestone Nov 30, 2023
@cscheid
Copy link
Collaborator

cscheid commented Dec 4, 2023

@cderv Could this be a knitr or rmd.R bug? Here's what's happening. If execute: false is present at the document level, then knitr doesn't include the OJS cells, even if they have include: true in them. The keep-md output looks like this:

---
format:
  html:
    code-fold: true
keep-md: true
execute:
  eval: true
  echo: true
  message: false
  warning: false
  output: true
  include: false
---







::: {.panel-tabset}

## Graph





## Data



:::


Even though the OJS cells have //| include: true in them.

@cscheid cscheid added the triaged-to Issues that were not self-assigned, signals that an issue was assigned to someone. label Dec 4, 2023
@cderv
Copy link
Collaborator

cderv commented Dec 5, 2023

If execute: false is present at the document level,

You mean include: false at top level right ?

Something is definitely not right in how ojs cell is handled. It seems knitr is not aware of the internal ojs cell option and this is why global default is not changed. I'll investigate what is happening.

Just to confirm: Are OJS cells handles after computation evaluation by quarto ? meaning after knitr ? What should be the output of the knitr step ? Should the //| options still be in the .md output or were they process before by Quarto ?

@cderv cderv removed the triaged-to Issues that were not self-assigned, signals that an issue was assigned to someone. label Dec 5, 2023
@cscheid
Copy link
Collaborator

cscheid commented Dec 5, 2023

Just to confirm: Are OJS cells handles after computation evaluation by quarto ? meaning after knitr ? What should be the output of the knitr step ? Should the //| options still be in the .md output or were they process before by Quarto ?

Yes, they're handled between engine execution and pandoc.

The problem here is that knitr would need to be able to recognize that an OJS code chunk has an include: true value, and then just forward it unchanged. I believe we have an "identity" hook for OJS blocks, but it doesn't look to be getting called here.

@cderv
Copy link
Collaborator

cderv commented Dec 5, 2023

I believe we have an "identity" hook for OJS blocks, but it doesn't look to be getting called here.

Yes we have that but with current implementation it is not just a hook but an engine which is supposed to output the same as entry. However, I believe it will be ignored because include: false is passed globally, and so knitr will not include the engine results unless include: true at the cell option level is found for that engine. This is the same for all kind of knitr engines

I do believe knitr can learn about OJS cell options - not an issue to add that to me in the parsing code.

However, I'll see if I can come up with another hook-type that would not be impacted by cell options like include.

But it is at the basis of knitr that engines to support echo, eval or include no matter what they output (identity or not).

@cscheid
Copy link
Collaborator

cscheid commented Dec 5, 2023

I'm sort of ok with this being a known bug for a while. When we add the "pre-knitr Pandoc pass" that we've talked about, then OJS cells will be completely handled before knitr is called, and this problem will go away.

yihui added a commit to yihui/knitr that referenced this issue Dec 6, 2023
…code for dot/mermaid/ojs chunks so that the full chunk content can be returned to Quarto
@cscheid cscheid added ojs-engine Issues with the ojs engine and removed observable-js labels Jan 31, 2024
clrpackages pushed a commit to clearlinux-pkgs/R-knitr that referenced this issue Apr 9, 2024
Kyle F Butts (2):
      Add support for `# %%` for chunk demarcation in `knitr::spin` (#2307)
      make spin() recognize `#|` comments as code chunks (#2320)

Lee Mendelowitz (1):
      Fix the error when kable()'s caption value is of length > 1 (#2312)

Max Schmit (1):
      add ALTER as sql statment, that is not returning anything (#2330)

Yihui Xie (53):
      start the next version
      fix #2304: shouldn't have used `return(meta)` since `meta` is not the `citation()` but `packageDescription()`
      fix rstudio/rmarkdown#2526 by reverting bebf117 since the `cairo_pdf` device has been explicitly supported in chunk_device(); now all possible `pdf()` arguments are supported, including `family`
      Disallow unbalanced chunk delimiters (#2306)
      close #1679: obtain the caption from the chunk option `tab.cap` for `kable()`
      also try the chunk option tbl.cap when tab.cap is NULL (quarto-dev/quarto-cli#7555)
      tweak R Markdown v1 vignettes
      move mailing list link and use https in ref card
      move css and js to jsdelivr for html_vignette
      use jsdelivr for the Rhtml template
      rewrite the docco classic style using JS instead of R
      standardize the docco linear format
      new version of funmediation is on CRAN now
      support the ... argument in chunk hooks
      optimize PNG images in package vignettes via optipng and pngquant if they are installed
      use R Markdown v1 for the knitr-intro vignette, too
      delete some files that are no longer needed in inst/examples and also ignore some files
      fix #2308: don't trim trailing spaces escaped by `\`
      support in-body chunk options for ojs and mermaid (quarto-dev/quarto-cli#7799)
      amend 3dcfac4780accc3791855b298a556297f62dc73d to fix quarto-dev/quarto-cli#5994: add YAML options to code for dot/mermaid/ojs chunks so that the full chunk content can be returned to Quarto
      add trailing slashes to URLs
      use the `assert(fact, {})` syntax for all tests
      revert c9473ad: map the chunk option `tbl-cap` to `tab.cap`
      leave tbl.cap alone; don't convert it to tab.cap, since Quarto has its own special handling of `tbl-cap`
      Create FUNDING.yml
      use xfun::decimal_dot() to ensure dot as the decimal separator
      fix rstudio/rmarkdown#2525: ensure the dot is used as the decimal separator even when options(OutDec) is not `.` or `LC_NUMERIC` is inappropriate
      factor out code to xfun::csv_options() and xfun::divide_chunk()
      get_option_comment() has been moved to xfun
      roxygenize
      bump xfun version
      xfun 0.42 is on CRAN now
      revert 87d094a6c593c31a3667821865e91b4a530018ec per request from CRAN maintainers, since _R_CHECK_VIGNETTES_SKIP_RUN_MAYBE_=true has been made the default
      fix rstudio/tinytex#435: adjust PATH temporarily for pdf_crop() to be able to find the command `pdfcrop` from TinyTeX
      RStudio IDE still calls parse_params(): https://github.com/rstudio/rstudio/blob/9a6196233d8eae5aa365b73eaafba04f7714190c/src/cpp/session/modules/SessionRmdNotebook.R#L666
      a follow-up on #2331: use xfun::fenced_block() from yihui/xfun@22a97ce
      xfun 0.43 is on CRAN now
      not sure what's wrong with pak
      `knitr::imgur_upload()` is now simply a wrapper function of `xfun::upload_imgur()` (#2325)
      too many ways to do the same thing... drop support for `#-` in spin()
      make spin() work again with input that can't be parsed as R code
      use xfun::check_old_package()
      markdown no longer sets this option internally, so no need to empty it now
      better detection of Rmd v2 input in knit2html(), so it no longer warns against documents that use markdown:: or litedown:: output formats
      make regex a little stricter so it won't match rmarkdown::
      the loo issue has been fixed (#2306)
      roxygen2 requires _PACKAGE now
      rename class names 'block' to 'knitr_block', and 'inline' to 'knitr_inline' since I need to export the print() methods and the names 'block' and 'inline' are too general
      preserve trailing \n in source code by doubling it, because later we will remove trailing \n in fenced_block()
      a temporary workaround for davidgohel/flextable#621
      StructFDR has been updated on CRAN but unfortunately the maintainer didn't seem to have seen my email about #2306
      I can't use knitr_block/knitr_inline as class names since it would break the lightparser package; let me just get rid of these S3 methods, which are unnecessarily advanced---just use normal functions instead (i.e., use inherits() to do the dispatch by myself)
      CRAN release v1.46

knokknok (1):
      fix #2318: faster processing of dependencies in dep_auto() (#2321)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working ojs-engine Issues with the ojs engine
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants