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

How to get formatting of 0.5.1 #90

Closed
ritchie46 opened this issue Sep 26, 2022 · 18 comments · Fixed by #92
Closed

How to get formatting of 0.5.1 #90

ritchie46 opened this issue Sep 26, 2022 · 18 comments · Fixed by #92
Labels
t: bug Something isn't working

Comments

@ritchie46
Copy link

We've tried updating comfytable in polars from 0.5.1 to 0.6.1 and only had to adapt a few method names, so that was great.

However the table formatting changed. This made all our doctests fail. Given that it is a huge undertaking to update all of them, I wonder if we can get the old formatting behavior.

An example of what we had:

    shape: (3, 3)
    ┌─────┬─────┬───────────┐
    │ a   ┆ b   ┆ b_squared │
    │ --- ┆ --- ┆ ---       │
    │ i64 ┆ i64 ┆ f64       │
    ╞═════╪═════╪═══════════╡
    │ 1   ┆ 2   ┆ 4.0       │
    ├╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌┤
    │ 3   ┆ 4   ┆ 16.0      │
    ├╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌╌╌┤
    │ 5   ┆ 6   ┆ 36.0      │
    └─────┴─────┴───────────┘

-----------------------------

And what we got after updating:

    shape: (3, 3)
    ┌─────┬─────┬─────────┐
    │ a   ┆ b   ┆ b_squar │
    │ --- ┆ --- ┆ ed      │
    │ i64 ┆ i64 ┆ ---     │
    │     ┆     ┆ f64     │
    ╞═════╪═════╪═════════╡
    │ 1   ┆ 2   ┆ 4.0     │
    ├╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
    │ 3   ┆ 4   ┆ 16.0    │
    ├╌╌╌╌╌┼╌╌╌╌╌┼╌╌╌╌╌╌╌╌╌┤
    │ 5   ┆ 6   ┆ 36.0    │
    └─────┴─────┴─────────┘

Any ideas?

@Nukesor
Copy link
Owner

Nukesor commented Sep 26, 2022

Could you link the code that generates this table?

It's rather hard to understand why this happens without some context 😅

Nukesor added a commit to Nukesor/Debug that referenced this issue Sep 26, 2022
@Nukesor
Copy link
Owner

Nukesor commented Sep 26, 2022

I just tried to create a minimal reproducable project for this issue, but wasn't able to do so.
You can find it over here:
https://github.com/Nukesor/debug/tree/comfy_table_90

I then updated comfy-table in the polars-core crate to get a better understanding on what's happening.

Here's my patch:

  diff --git a/polars/polars-core/Cargo.toml b/polars/polars-core/Cargo.toml
  index 420edc826..bb1e03b4c 100644
  --- a/polars/polars-core/Cargo.toml
  +++ b/polars/polars-core/Cargo.toml
  @@ -152,7 +152,7 @@ base64 = { version = "0.13", optional = true }
   bitflags = "1.3"
   chrono = { version = "0.4", optional = true }
   chrono-tz = { version = "0.6", optional = true }
  -comfy-table = { version = "5.0", optional = true }
  +comfy-table = { version = "6.1", optional = true }
   hashbrown.workspace = true
   hex = { version = "0.4", optional = true }
   indexmap = { version = "1", features = ["std"] }
  @@ -160,7 +160,9 @@ jsonpath_lib = { version = "0.3.0", optional = true, git = "https://github.com/r
   ndarray = { version = "0.15", optional = true, default_features = false }
   num.workspace = true
   once_cell = "1"
   polars-arrow = { version = "0.24.2", path = "../polars-arrow", features = ["compute"] }
   polars-utils = { version = "0.24.2", path = "../polars-utils" }
   rand = { version = "0.8", optional = true, features = ["small_rng", "std"] }
   rand_distr = { version = "0.4", optional = true }
  diff --git a/polars/polars-core/src/fmt.rs b/polars/polars-core/src/fmt.rs
  index fa14e1aab..7bad3f5a5 100644
  --- a/polars/polars-core/src/fmt.rs
  +++ b/polars/polars-core/src/fmt.rs
  @@ -469,15 +469,21 @@ impl Display for DataFrame {
                   .unwrap_or(None);
               // if tbl_width is explicitly set, use it
               if let Some(w) = tbl_width {
  -                table.set_table_width(w);
  +                table.set_width(w);
               }

  +            dbg!("Table width:");
  +            dbg!(table.width());
  +
               // if no tbl_width (its not-tty && it is not explicitly set), then set default
               // this is needed to support non-tty applications
  -            if !table.is_tty() && table.get_table_width().is_none() {
  -                table.set_table_width(100);
  +            if !table.is_tty() && table.width().is_none() {
  +                table.set_width(100);
               }

  +            dbg!("Table width:");
  +            dbg!(table.width());
  +
               // set alignment of cells if defined

               if std::env::var("POLARS_FMT_TABLE_CELL_ALIGNMENT").is_ok() {

Running cargo test --features=all doesn't come up with any test errors though.

@ritchie46
Copy link
Author

Thanks for the help. Note that there are no errors. The tables formatting does work, it only has changed. In the example above b_squared got wrapped to a new line.

The tests that fail are the doctests on the python side. They check the output of docstring examples and are thus sensitive to formatting. This is terrible as it is a lot of work to update very method example. 😅

Do you know which settings influences this wrapping? Maybe I can fiddle around.

@Nukesor
Copy link
Owner

Nukesor commented Sep 26, 2022

Small syntactic sugar:
https://github.com/pola-rs/polars/blob/1a4f0f36b74621d5360959b256254c9fa9d3bfc2/polars/polars-core/src/fmt.rs#L440

Table now implements add_rows

                for row in rows {
                    table.add_row(row);
                }

-> `table.add_rows(rows);

@Nukesor
Copy link
Owner

Nukesor commented Sep 26, 2022

I just managed to reproduce it! Nice.
I'll now try to find out why this happens.

@Nukesor
Copy link
Owner

Nukesor commented Sep 26, 2022

Could you check whether the fixes in #92 fix your problem at hand?

I actually found two formatting edge-cases while writing test.

@Nukesor Nukesor added the t: bug Something isn't working label Sep 26, 2022
@Nukesor
Copy link
Owner

Nukesor commented Oct 2, 2022

Ping @ritchie46

@ritchie46
Copy link
Author

Thanks! And sorry for my delay. I tried the branch in #92, but still have different formatting:

example:

    )
Expected:
    shape: (6, 1)
    ┌──────┐
    │ A    │
    │ ---  │
    │ f64  │
    ╞══════╡
    │ null │
    ├╌╌╌╌╌╌┤
    │ 2.0  │
    ├╌╌╌╌╌╌┤
    │ 3.0  │
    ├╌╌╌╌╌╌┤
    │ 4.0  │
    ├╌╌╌╌╌╌┤
    │ 5.0  │
    ├╌╌╌╌╌╌┤
    │ 6.0  │
    └──────┘
Got:
    shape: (6, 1)
    ┌─────┐
    │ A   │
    │ --- │
    │ f64 │
    ╞═════╡
    │ nul │
    │ l   │
    ├╌╌╌╌╌┤
    │ 2.0 │
    ├╌╌╌╌╌┤
    │ 3.0 │
    ├╌╌╌╌╌┤
    │ 4.0 │
    ├╌╌╌╌╌┤
    │ 5.0 │
    ├╌╌╌╌╌┤
    │ 6.0 │
    └─────┘

@ritchie46
Copy link
Author

Here is the updating PR: pola-rs/polars#5077

@Nukesor
Copy link
Owner

Nukesor commented Oct 3, 2022

Thanks for sharing that MR.
I managed to get the python setup working on my local machine. I get the same error as your CI, i.e. test_tbl_width_chars.

With that, I should be able to debug this until everything works as expected.

Still. I don't see the error you posted above. That example works perfectly fine and also doesn't show up in the CI test failure.
Do you get this test failure in your local dev environment?

@ritchie46
Copy link
Author

Yes, if we go to polars/py-polars and then run $ make doctest, the python doctests will run. Those doctests check the printed output with the output hardcoded in the examples docstrings.

Here is an example of such a docstring with a hard-coded table: https://github.com/pola-rs/polars/blob/49898c62f27335cf8fe33f7081dad26ab02a38ec/py-polars/polars/internals/dataframe/frame.py#L2734

@Nukesor
Copy link
Owner

Nukesor commented Oct 3, 2022

Thanks!

@Nukesor
Copy link
Owner

Nukesor commented Oct 13, 2022

I'm closing in on the issue.

I'll try to get get the proptest test suite up and running before considering this as fixed. I don't like that this many issues managed to slip in, since I disabled the proptest suite (I had to temporarily do this because of #53)

@Nukesor
Copy link
Owner

Nukesor commented Oct 13, 2022

Found the issue. I introduced a bug when refactoring the content_width calculation for columns in 6.0.

The issue has been fixed in b5f8e76 .

As I said above, I'll still try to get the proptest suite up and running again and push a point release once I'm confident that everything's fine.

The py-polars python test-suite now seems to succeed with the latest branch head.

@ritchie46
Copy link
Author

That's great to hear! You have saved us many hours of work! 🙌

@Nukesor
Copy link
Owner

Nukesor commented Oct 18, 2022

I just published a point release :)

You should be good to go on that update.

@ritchie46
Copy link
Author

I just published a point release :)

You should be good to go on that update.

Can confirm it works. 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
t: bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants