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

feat: add knit_print method for DataFrame (experimental) #125

Merged
merged 36 commits into from
Apr 27, 2023

Conversation

eitsupi
Copy link
Collaborator

@eitsupi eitsupi commented Apr 18, 2023

Add the ability to output pretty HTML tables on knitr documents.

@eitsupi eitsupi marked this pull request as ready for review April 18, 2023 16:01
@eitsupi
Copy link
Collaborator Author

eitsupi commented Apr 18, 2023

I am confused about the errors in the test.
Locally the test does not fail.

R/pkg-knitr.R Outdated Show resolved Hide resolved
tests/testthat/test-knitr.R Outdated Show resolved Hide resolved
R/pkg-knitr.R Outdated Show resolved Hide resolved
R/pkg-knitr.R Outdated Show resolved Hide resolved
@eitsupi eitsupi requested a review from sorhawell April 18, 2023 16:12
@sorhawell
Copy link
Collaborator

I am confused about the errors in the test. Locally the test does not fail.

I think I have had that error before also, I can try fiddle with it this evening.

@sorhawell
Copy link
Collaborator

@eitsupi I also updated the snapshot, which had slightly different spacings on my machine. At worst polars could have some machine-specific inference of line widths which is good for users and bad for testing. In such case, maybe we can use a DataFrame with only 1-3 columns as test case.

@eitsupi
Copy link
Collaborator Author

eitsupi commented Apr 18, 2023

Thanks!

Definitely a small data frame seems to be the right target for snapshot testing here. I will update.

@eitsupi eitsupi marked this pull request as draft April 18, 2023 23:33
these flagments don't support `POLARS_FMT_TABLE_DATAFRAME_SHAPE_BELOW=1`
@eitsupi
Copy link
Collaborator Author

eitsupi commented Apr 19, 2023

Sorry, but I found that the current approach will fail if there is a column of list or long column name.
We can check with dplyr::starwars for example.

> Sys.setenv(POLARS_FMT_TABLE_FORMATTING = "ASCII_MARKDOWN")

> Sys.setenv(POLARS_FMT_TABLE_INLINE_COLUMN_DATA_TYPE = "1")

> pl$DataFrame(dplyr::starwars)
shape: (87, 14)
| name (str)     | height (i32) | mass (f64) | hair_color | ... | species (str) | films          | vehicles       | starships     |
|                |              |            | (str)      |     |               | (list[str])    | (list[str])    | (list[str])   |
|----------------|--------------|------------|------------|-----|---------------|----------------|----------------|---------------|
| Luke Skywalker | 172          | 77.0       | blond      | ... | Human         | ["The Empire   | ["Snowspeeder" | ["X-wing",    |
|                |              |            |            |     |               | Strikes Back", | , "Imperial    | "Imperial     |
|                |              |            |            |     |               | "Rev...        | Speede...      | shuttle"]     |
| C-3PO          | 167          | 75.0       | null       | ... | Droid         | ["The Empire   | []             | []            |
|                |              |            |            |     |               | Strikes Back", |                |               |
|                |              |            |            |     |               | "Att...        |                |               |
| R2-D2          | 96           | 32.0       | null       | ... | Droid         | ["The Empire   | []             | []            |
|                |              |            |            |     |               | Strikes Back", |                |               |
|                |              |            |            |     |               | "Att...        |                |               |
| Darth Vader    | 202          | 136.0      | none       | ... | Human         | ["The Empire   | []             | ["TIE         |
|                |              |            |            |     |               | Strikes Back", |                | Advanced x1"] |
|                |              |            |            |     |               | "Rev...        |                |               |
| ...            | ...          | ...        | ...        | ... | ...           | ...            | ...            | ...           |
| Poe Dameron    | null         | null       | brown      | ... | Human         | ["The Force    | []             | ["T-70 X-wing |
|                |              |            |            |     |               | Awakens"]      |                | fighter"]     |
| BB8            | null         | null       | none       | ... | Droid         | ["The Force    | []             | []            |
|                |              |            |            |     |               | Awakens"]      |                |               |
| Captain Phasma | null         | null       | unknown    | ... | null          | ["The Force    | []             | []            |
|                |              |            |            |     |               | Awakens"]      |                |               |
| Padmé Amidala  | 165          | 45.0       | brown      | ... | Human         | ["Attack of    | []             | ["H-type      |
|                |              |            |            |     |               | the Clones",   |                | Nubian        |
|                |              |            |            |     |               | "The Ph...     |                | yacht",       |
|                |              |            |            |     |               |                |                | "Naboo s...   |

Perhaps we may need to write out the HTML in the same way as python-polars...
https://github.com/pola-rs/polars/blob/81b4102ac03f79361645e709f05061d9bb876a51/py-polars/polars/dataframe/_html.py#L48-L115

@eitsupi eitsupi mentioned this pull request Apr 19, 2023
This was referenced Apr 20, 2023
Copy link
Collaborator

@sorhawell sorhawell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eitsupi
colname and value(s) are right-aligned, whereas colname and type are left-aligned. Would it look good to right-align all three (colname, type, value(s))?

... or left align

image

@eitsupi
Copy link
Collaborator Author

eitsupi commented Apr 23, 2023

@eitsupi colname and value(s) are right-aligned, whereas colname and type are left-aligned. Would it look good to right-align all three (colname, type, value(s))?

... or left align

Good point, that bothered me too.

But I think this looks exactly the same as the current py-polars.
For example, this is what happens when we render the following Rmd file.

---
title: test
output:
  github_document:
    df_print: paged
---

```{r}
#| include: false
nycflights13::flights[1, ] |> arrow::write_parquet("flights.parquet")
devtools::load_all()
```

```{python}
import polars as pl

pl.read_parquet("flights.parquet")
```

```{r}
polars::scan_parquet("flights.parquet")$collect()
```

image
image

(Only the R's have [ and ] escaped with a backslash.......needs to be fixed.)

On the other hand, the following qmd file is right-aligned when rendered by Quarto CLI (1.3, prerelease version).

---
title: test
format:
  html:
    self-contained: true
    df-print: paged
---

```{r}
#| include: false
nycflights13::flights[1, ] |> arrow::write_parquet("flights.parquet")
devtools::load_all()
```

```{python}
import polars as pl

pl.read_parquet("flights.parquet")
```

```{r}
polars::scan_parquet("flights.parquet")$collect()
```

image
image

So this is more like a CSS (?) problem of rmarkdown package.

@eitsupi eitsupi requested a review from sorhawell April 24, 2023 10:41
@eitsupi
Copy link
Collaborator Author

eitsupi commented Apr 24, 2023

Fixed to look exactly the same as python-polars!

image

R/pkg-knitr.R Outdated Show resolved Hide resolved
R/pkg-knitr.R Outdated Show resolved Hide resolved
R/pkg-knitr.R Outdated Show resolved Hide resolved
Copy link
Collaborator

@sorhawell sorhawell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks fine to me
Worked as expected on my machine

Copy link
Collaborator

@vincentarelbundock vincentarelbundock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. This is a very nice improvement!

@eitsupi eitsupi merged commit 250b660 into pola-rs:main Apr 27, 2023
8 checks passed
@eitsupi eitsupi deleted the knitr-print branch April 27, 2023 12:52
vincentarelbundock pushed a commit to vincentarelbundock/r-polars that referenced this pull request Apr 27, 2023
vincentarelbundock pushed a commit to vincentarelbundock/r-polars that referenced this pull request Apr 29, 2023
vincentarelbundock pushed a commit to vincentarelbundock/r-polars that referenced this pull request Apr 29, 2023
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.

None yet

3 participants