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

Fix list struct print bug #1027

Merged
merged 4 commits into from
Nov 27, 2024
Merged

Conversation

billylanchantin
Copy link
Contributor

Part of: #1014

Fixes an issue where we can't print columns with a dtype like {:list, {:struct, ...}} where the root of the tree isn't a :struct but it contains a :struct. The cause is that we're not accounting for nested structs in our print function:

|> Enum.map(fn n ->
s = df[n]
list = Series.to_list(s)
case s.dtype do
{:struct, _} -> Enum.map(list, &inspect/1)
_ -> list
end
end)

The solution is a recursive function.

I've also made some additional choices:

  1. Contents of structs now look the same inside and outside a struct.
    Before we were calling inspect/1. That resulted in e.g. strings getting "" delimiters. Strings don't usually get "" delimiters when printed in a dataframe.
  2. Lists now get [] delimiters.
    This is especially helpful in identifying empty lists and singleton lists. Before these cases were a bit confusing.
  3. Structs now get {} delimiters instead of %{}.
  4. Keys of structs go from "x" => to x:.
  5. nil is now represented as the literal nil instead of an empty string.
  6. We now intent one space when nesting.

Overall I found these choices looked much better. Here's just one example of the new format:

+--------------------------------------------------------+
|       Explorer DataFrame: [rows: 3, columns: 2]        |
+--------------------------------+-----------------------+
|             col_d              |         col_j         |
|          <struct[2]>           |   <list[list[f32]]>   |
+================================+=======================+
| {                              | [                     |
|  q: 2901-09-24 22:53:38.923913 |  [                    |
|  w: 3470490443432017527        |   1.100000023841858   |
| }                              |   2.200000047683716   |
|                                |  ]                    |
|                                | ]                     |
+--------------------------------+-----------------------+
| {                              | [[3.299999952316284]] |
|  q: nil                        |                       |
|  w: nil                        |                       |
| }                              |                       |
+--------------------------------+-----------------------+
| {                              | [                     |
|  q: nil                        |  nil                  |
|  w: 3594148901191965499        |  [nil]                |
| }                              |  []                   |
|                                | ]                     |
+--------------------------------+-----------------------+

Copy link
Member

@josevalim josevalim left a comment

Choose a reason for hiding this comment

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

All is good to me!

Keys of structs go from "x" => to x:

The idea would be to show what it would actually be used if they were converted to Elixir, but either way is fine!

@billylanchantin
Copy link
Contributor Author

I decided on x: over "x" => because 1) horizontal space is limited and 2) we don't use literals elsewhere. E.g. strings are my string not "my string"

If later anyone feels strongly about these choices though I'm happy to walk them back!

@billylanchantin billylanchantin merged commit 9519021 into main Nov 27, 2024
3 checks passed
@billylanchantin billylanchantin deleted the bl-fix-list-struct-print-bug branch November 27, 2024 22:08
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.

2 participants