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

Inline code chunks inheriting labels #1988

Closed
3 tasks done
rundel opened this issue Apr 22, 2021 · 7 comments · Fixed by #2292
Closed
3 tasks done

Inline code chunks inheriting labels #1988

rundel opened this issue Apr 22, 2021 · 7 comments · Fixed by #2292
Labels
help needed We need your help with the issue/PR

Comments

@rundel
Copy link

rundel commented Apr 22, 2021

It seems as though the label behavior for inline code chunks is inconsistent / problematic - specifically it seems that they inherit the label of the most recent proceeding code chunk, and in the case where an inline chunk occurs before any other it will have a blank label. See the sample Rmd below which demonstrates these behaviors:

---
output: html_document
---

Inline - "`r knitr::opts_current$get("label")`"

# Unlabeled chunk

```{r}
knitr::opts_current$get("label")
```

Inline - "`r knitr::opts_current$get("label")`"

Inline - "`r knitr::opts_current$get("label")`"

# Label chunk

```{r label}
knitr::opts_current$get("label")
```

Inline - "`r knitr::opts_current$get("label")`"


# Different engines

```{python pychunk}
1+1
```

Inline - "`r knitr::opts_current$get("label")`"

I've come across a specific issue with this when using an inline code chunk to generate an image (via magick) as the plot_counter() is reset between the regular and inline chunk but since they share a label the plots from earlier regular chunk are overwritten.

I would think that the more desireable behavior would be for the chunk label to increment regardless of the chunk type, the labels could continue to use unnamed-chunk-* or could have a specific inline chunk label variant e.g. inline-chunk-1, inline-chunk-2, etc.


By filing an issue to this repo, I promise that

  • I have fully read the issue guide at https://yihui.org/issue/.
  • I have provided the necessary information about my issue.
  • I have learned the Github Markdown syntax, and formatted my issue correctly.
xfun::session_info('knitr')
## R version 4.0.5 (2021-03-31)
## Platform: x86_64-apple-darwin20.3.0 (64-bit)
## Running under: macOS Big Sur 10.16, RStudio 1.4.1114
## 
## Locale: en_US.UTF-8 / en_US.UTF-8 / en_US.UTF-8 / C / en_US.UTF-8 / ## en_US.UTF-8
## 
## Package version:
##   evaluate_0.14   glue_1.4.2      graphics_4.0.5  grDevices_4.0.5
##   highr_0.9       knitr_1.32      magrittr_2.0.1  markdown_1.1   
##   methods_4.0.5   mime_0.10       stats_4.0.5     stringi_1.5.3  
##   stringr_1.4.0   tools_4.0.5     utils_4.0.5     xfun_0.22      
##   yaml_2.2.1  
@cderv
Copy link
Collaborator

cderv commented Apr 22, 2021

Thanks for the report!

I think the original issue in magick is important here to understand.

This happens because magick:::knit_print.magick-image` uses internal knitr function to save to a file
https://github.com/ropensci/magick/blob/56f746be5edb3a9e7c40b2c91ef3bd0b51bbb0f1/R/base.R#L117-L131

"knit_print.magick-image" <- function(x, ...){
  if(!length(x))
    return(invisible())
  plot_counter <- utils::getFromNamespace('plot_counter', 'knitr')
  in_base_dir <- utils::getFromNamespace('in_base_dir', 'knitr')
  ext <- ifelse(all(tolower(image_info(x)$format) == "gif"), "gif", "png")
  tmp <- knitr::fig_path(ext, number = plot_counter())

  # save relative to 'base' directory, see discussion in #110
  in_base_dir({
    dir.create(dirname(tmp), showWarnings = FALSE, recursive = TRUE)
    image_write(x, path = tmp, format = ext)
  })
  knitr::include_graphics(tmp)
}

Currently this method, does not make any differences between inline and block chunk as it could (see inline argument for knit_print)
and it is calling plot_counter() which is something used internally to create figure name based on label in each chunk.
Currently, I don't think this is expected to call this function for inline chunk. In knitr, I believe plot_counter() is called only for block chunk evaluation, and the counter is reset after each chunk, because label is supposed to be set for every block chunk. But there is no label for inline chunk.

I don't think we expect inline chunk to create figures as it is used to create plot files for plot evaluated by knitr. And you won't call plot(...) in an inline chunk I believe.

So this is currently due to magick using an internal feature of knitr that does not work for inline chunk.

However, I understand the need to have file created for inline insertion, specifically with svg. fontawesome does that, but it creates the filename itself without using knitr internal counter:
https://github.com/rstudio/fontawesome/blob/23e89f30ad26b1296bf5ec86c335db1d8de25ade/R/knit_print.R#L44-L49

So either magick could deal with inline figure differently, or create filemane differently.
Or we could try to provide this kind of support in knitr for inline chunk (like a label and a counter) as it seems to be useful for inserting inline image.

@rundel
Copy link
Author

rundel commented Apr 22, 2021

Thanks for the quick reply - my 2 cents (coming from someone who is not intimately familiar with the knitr internals) is that having fewer distinctions between inline and regular chunks will make things better for other developers integrating with knitr.

For example, I realize that not all chunk options necessarily make sense but clearly the current "rule" that inline chunks don't have labels isn't true as knitr::opts_current$get("label") returns something, hence the current issue. This could be addressed with the incremental labeling solution mentioned above, or at least throwing an error if accessing an "undefined" option.

I think the larger issue is that the current implementation of inline chunks is such that I as a developer don't have any mental model about where inline chunk settings will come from - global settings, inherited from previous chunk(s), some inline only defaults, etc. While I haven't tested or looked at the code for these other option and so I'm not sure about the details, I am worried that potentially non-obvious but related issues will happen down the road because of these inconsistencies between the way the chunks are treated.

@cderv
Copy link
Collaborator

cderv commented Apr 22, 2021

Thanks for your feedback. The mental model question is really interesting.

As I understand, Inline R code and block chunk does not work the same currently. There is no options for inline R code and so no label. You cannot even change engine for inline code. It is only R code. So I guess my mental model for now is that any block chunk related functions won't work correctly in an inline R code. And knitr::opts_current is a mechanism to get the current option of a chunk. So I currently expect it not to work as inline R code

I don't know if maybe documentation should be made more clear that inline R code are not chunks (with options and all), and functions for chunk would error on inline us.
or if maybe we should think broader about making inline R code work more closely to block chunks.

I believe your suggestion are related to the latter: Setting a specific label for inline R code suppose it can have a label. And keep incrementing no matter the type supposes that inline execution knows about the counter - which they don't.

I am thinking as I am writing and share it here for when I'll come back to this.
Thank you very much for this discussion that brings interesting questions!

@rundel
Copy link
Author

rundel commented Apr 23, 2021

It seems like a missed opportunity to not have them work in the same way - I'm not a regular python user but it would be somewhat frustrating to not be able to use python inline or have to do something convoluted through reticulate. Additionally, while having image output is probably the most compelling use case here I can also imagine edge cases like wanting to have an equivalent of error = TRUE to be able to demonstrate an error message inline.

Clearly these are mostly marginal use cases and would require a not insubstantial amount of work to implement but I think there is some value there.

@rundel
Copy link
Author

rundel commented Apr 28, 2021

After working through the PR for magick ropensci/magick#313, I'm back to feeling like this is something that needs to be fixed on the knitr side of things.

The primary cause of the issue here is the knitr::fig_path() function is broken for inline chunks - since it is exported and documented I would think it is fair game for other packages to use and at a minimum if using it in an inline chunk is not allowed then there needs to be some guardrails on knitr's side of things.

There are deeper issues here around the scope of knitr::opt_current persisting after a chunk, and this may be causing other bug / unexpected behaviors with functions like knitr::fig_path() when used from an inline chunk. So maybe the check for "inline" needs to be done around this as a short term fix.

Work arounds can be made in magick, rsicons, etc. but I think the ultimate fix is needed in knitr.

@yihui
Copy link
Owner

yihui commented Jun 26, 2021

I would think that the more desireable behavior would be for the chunk label to increment regardless of the chunk type, the labels could continue to use unnamed-chunk-* or could have a specific inline chunk label variant e.g. inline-chunk-1, inline-chunk-2, etc.

That sounds reasonable to me.

Work arounds can be made in magick, rsicons, etc. but I think the ultimate fix is needed in knitr.

I didn't read this thread very carefully, but I'll be happy to have this problem fixed in knitr. It will be great if someone could send a pull request, although I don't be able to review it by myself since I'll be on vacation starting tomorrow till mid-July. Perhaps @cderv can help review it (if it's not too complicated, you can merge it without my review).

Thanks!

@yihui yihui added the help needed We need your help with the issue/PR label Mar 24, 2022
yihui added a commit that referenced this issue Oct 25, 2023
… exits (#2292)

also add labels to `opts_current` for inline code to fix #1988
yihui added a commit that referenced this issue Oct 26, 2023
… exits (#2292)

also add labels to `opts_current` for inline code to fix #1988
Copy link

This old thread has been automatically locked. If you think you have found something related to this, please open a new issue by following the issue guide (https://yihui.org/issue/), and link to this old issue if necessary.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
help needed We need your help with the issue/PR
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants