add displaycoltypes to showrows & show for dataframes #2142

ssikdar1 · 2020-03-05T18:31:04Z

For issue #2116 to have the types in the dataframe heading to be optional.

Added a boolean type displaycoltypes for showrows and coltypes tor show in src/abstractdataframe/show.jl

Testing

In the REPL:

$ ./julia 
               _
   _       _ _(_)_     |  Documentation: https://docs.julialang.org
  (_)     | (_) (_)    |
   _ _   _| |_  __ _   |  Type "?" for help, "]?" for Pkg help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 1.5.0-DEV.384 (2020-03-02)
 _/ |\__'_|_|_|\__'_|  |  inline/b8ddf2a811* (fork: 6 commits, 4 days)
|__/                   |

julia> using DataFrames
[ Info: Precompiling DataFrames [a93c6f00-e57d-5684-b7b6-d8193f3e46c0]

julia> df = DataFrame(A = 1:3, B = ["x", "y", "z"]);

julia> DataFrames.showrows(stdout, df, 1:2, 3:3, [5, 6, 3], false, true, :Row, true, false)
3×2 DataFrame
│ Row │ A     │ B      │
├─────┼───────┼────────┤
│ 1   │ 1     │ x      │
│ 2   │ 2     │ y      │
⋮
│ 3   │ 3     │ z      │
julia> DataFrames.showrows(stdout, df, 1:2, 3:3, [5, 6, 3], false, true, :Row, true, true)
3×2 DataFrame
│ Row │ A     │ B      │
│     │ Int64 │ String │
├─────┼───────┼────────┤
│ 1   │ 1     │ x      │
│ 2   │ 2     │ y      │
⋮
│ 3   │ 3     │ z      │
julia> show(df, coltypes=false)
3×2 DataFrame
│ Row │ A     │ B      │
├─────┼───────┼────────┤
│ 1   │ 1     │ x      │
│ 2   │ 2     │ y      │
│ 3   │ 3     │ z      │
julia> show(df, coltypes=true)
3×2 DataFrame
│ Row │ A     │ B      │
│     │ Int64 │ String │
├─────┼───────┼────────┤
│ 1   │ 1     │ x      │
│ 2   │ 2     │ y      │
│ 3   │ 3     │ z      │
julia>

src/abstractdataframe/show.jl

bkamins · 2020-03-05T20:06:44Z

Thank you for the contribution.

I will review this PR after #2140 is merged (to avoid duplicate work on your side).
After #2140 is merged can you then please rebase this PR and add tests (now they are missing).

The one thing that should be fixed in the PR is that when we omit printing eltypes then eltype row should not be considered when calculating column width (now you only disabled printing it, but still it is considered to determine with of the column - which is visible in your examples).

nalimilan

Thanks, I just have a few small comments, I'll leave it to @bkamins to check implementation tricks.

Regarding the name, maybe it should be eltypes, as these are not really the types of columns themselves?

nalimilan · 2020-03-07T19:08:28Z

src/abstractdataframe/show.jl

@@ -607,6 +619,7 @@ while `splitcols` defaults to `true`.
  By default this is the case only if `io` has the `IOContext` property `limit` set.
 - `rowlabel::Symbol = :Row`: The label to use for the column containing row numbers.
 - `summary::Bool = true`: Whether to print a brief string summary of the data frame.
+- `coltypes::Bool = true`: Whether to print the column types under column names in data frame.


Suggested change

- `coltypes::Bool = true`: Whether to print the column types under column names in data frame.

- `coltypes::Bool = true`: Whether to print the column types under column names.

Addressed in 052de9a

nalimilan · 2020-03-07T19:08:49Z

src/abstractdataframe/show.jl

@@ -390,6 +395,8 @@ NOTE: The value of `maxwidths[end]` must be the string width of
 - `displaysummary::Bool`: Should a brief string summary of the
  AbstractDataFrame be rendered to the I/O stream before printing the
  contents of the renderable rows? Defaults to `true`.
+- `displaycoltype::Bool = true`: Whether to print the column type
+   under the column name in the heading. Defaults to true.


Suggested change

under the column name in the heading. Defaults to true.

under the column name in the heading. Defaults to `true`.

Addressed in 052de9a

nalimilan · 2020-03-07T19:10:20Z

src/abstractdataframe/show.jl

@@ -131,7 +132,6 @@ function getmaxwidths(df::AbstractDataFrame,
    for (name, col) in pairs(eachcol(df))
        # (1) Consider length of column name
        maxwidth = ourstrwidth(io, name)
-


Unrelated, please revert.

Addressed in 052de9a

bkamins · 2020-03-07T23:16:51Z

OK - I will have a final look when the changes by @nalimilan are incorporated.

ssikdar1 · 2020-03-09T13:12:37Z

okay, @nalimilan comments should have been addressed in 052de9a

bkamins · 2020-03-09T13:30:26Z

OK. As I have expected you have merge conflicts now due to #2140.

Can you please rebase the PR and then the following things should be done before it is merged:

syncing HTML and LaTeX output to also support the kwarg (we have to do it so that code is portable across backends)
adding tests for text/plain, HTML, and LaTeX output without column names
optionally if you wish adding an entry in the manual informing users that there is an option to disable printing of the eltypes

I hope this list is not too long, but we need to keep things in sync with displaying things (and astonishingly these things tend to be more complex than they seem to be).

… show

bkamins · 2020-03-09T22:30:37Z

Thank you for the update.

What about the comment of @nalimilan to use eltypes as the name of the kwarg? Also it would be easier to maintain the code in the long run if the same name of the argument were used consistently across all functions (even if in other cases it is not so - probably for legacy reasons).

Other than that - the PR looks good. Can you just please check if we correctly adjust the number of rows to be printed (if we do not print eltypes then we have one more line available)?

And we still need tests and possibly some manual entry.

ssikdar1 · 2020-03-10T00:57:03Z

@bkamins sorry! I pushed without thinking! Give me a couple more days and ill try to get this in order.

bkamins · 2020-03-10T06:49:24Z

No problem. Thank you for working on that!

ssikdar1 · 2020-03-10T13:38:35Z

@bkamins the most recent commits should hopefully address the issues:

rebase the PR & merge [X]
standardize names to eltypes [X]
syncing HTML and LaTeX output
- text/plain [X]
- text/html [X]
- text/latex [X]
adding tests for text/plain, HTML, and LaTeX output without column names
- show [X]
- text/plain [X]
- text/html [X]
- textlatex [X]
optionally if you wish adding an entry in the manual informing users that there is an option to disable printing of the eltypes
- add entry []
  Not sure where to put this?

test/io.jl

src/abstractdataframe/io.jl

src/abstractdataframe/show.jl

test/io.jl

bkamins · 2020-03-10T19:57:51Z

Thank you. Apart from comments we should have tests for text/plain, HTML and LaTeX for DataFrameRow, DataFrameRows and DataFrameColumns types with eltypes=false.

Thank you!

bkamins · 2020-03-11T22:57:27Z

OK - so we are left with adding DataFrameRow, DataFrameRows and DataFrameColumns tests. If you are not clear how to write them then please let me know. We will merge this PR and I will open another only with these tests.

ssikdar1 · 2020-03-12T04:20:19Z

OK - so we are left with adding DataFrameRow, DataFrameRows and DataFrameColumns tests. If you are not clear how to write them then please let me know. We will merge this PR and I will open another only with these tests.

Yes sorry about this! please go ahead and make another PR!

bkamins · 2020-03-12T07:25:36Z

@nalimilan - I think we can merge it if you are OK with this PR.
After this is merged I will open another PR which will:

add missing tests
clean up the implementation

src/abstractdataframe/io.jl

src/abstractdataframe/show.jl

test/show.jl

test/io.jl

nalimilan

I've committed a few suggestions. Looks good to me, thanks!

bkamins · 2020-03-12T10:41:27Z

Thank you!

add displaycoltypes to showrows show for dataframes

22fa765

bkamins reviewed Mar 5, 2020

View reviewed changes

src/abstractdataframe/show.jl Outdated Show resolved Hide resolved

bkamins added the non-breaking The proposed change is not breaking label Mar 5, 2020

bkamins added this to the 1.x milestone Mar 5, 2020

fix column width (eltypes calc) for displaycolumn bool

d0df307

nalimilan reviewed Mar 7, 2020

View reviewed changes

update show.jl w/ code review comments

052de9a

ssikdar1 added 4 commits March 9, 2020 16:48

add displaycoltypes to showrows show for dataframes

4890c3a

Merge remote-tracking branch 'upstream/master' into show

0f67963

add coltypes to text/html show

afec4ca

Merge branch 'show' of https://github.com/ssikdar1/DataFrames.jl into…

8485db6

… show

ssikdar1 added 4 commits March 10, 2020 08:37

change coltypes/displaycoltypes => eltypes

41b335b

add eltypes to text/latex

cd4219f

add eltypes to text/plain

b31502b

add tests for show plain latex html

ebaf0b5

bkamins reviewed Mar 10, 2020

View reviewed changes

test/io.jl Outdated Show resolved Hide resolved

ssikdar1 added 2 commits March 10, 2020 11:43

fix testset block

4ac1217

cast dataframe test column to Int32 to work both on x86 and 64 bit tests

5c904e4

bkamins reviewed Mar 10, 2020

View reviewed changes

src/abstractdataframe/io.jl Outdated Show resolved Hide resolved

bkamins reviewed Mar 10, 2020

View reviewed changes

src/abstractdataframe/show.jl Outdated Show resolved Hide resolved

bkamins reviewed Mar 10, 2020

View reviewed changes

test/io.jl Outdated Show resolved Hide resolved

ssikdar1 added 4 commits March 10, 2020 16:27

fix show_eltype formatting

3a7b031

wrap tests strings in triple quotes instead of single quotes

4bb6529

split functions onto multiple lines

de7e4e9

improve the visual formatting of the strings in io tests

a9d733f

bkamins approved these changes Mar 12, 2020

View reviewed changes

nalimilan reviewed Mar 12, 2020

View reviewed changes

src/abstractdataframe/io.jl Outdated Show resolved Hide resolved

src/abstractdataframe/show.jl Outdated Show resolved Hide resolved

test/show.jl Outdated Show resolved Hide resolved

test/io.jl Outdated Show resolved Hide resolved

Apply suggestions from code review

aaf4376

nalimilan approved these changes Mar 12, 2020

View reviewed changes

bkamins merged commit 61ecf44 into JuliaData:master Mar 12, 2020

bkamins mentioned this pull request Mar 12, 2020

finalize adding eltypes to show #2151

Merged

ssikdar1 deleted the show branch March 12, 2020 13:51

bkamins mentioned this pull request Mar 21, 2020

Optionally remove type from heading #2116

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add displaycoltypes to showrows & show for dataframes #2142

add displaycoltypes to showrows & show for dataframes #2142

ssikdar1 commented Mar 5, 2020

bkamins commented Mar 5, 2020

nalimilan left a comment

nalimilan Mar 7, 2020

ssikdar1 Mar 8, 2020

nalimilan Mar 7, 2020

ssikdar1 Mar 8, 2020

nalimilan Mar 7, 2020

ssikdar1 Mar 8, 2020

bkamins commented Mar 7, 2020

ssikdar1 commented Mar 9, 2020

bkamins commented Mar 9, 2020

bkamins commented Mar 9, 2020

ssikdar1 commented Mar 10, 2020

bkamins commented Mar 10, 2020

ssikdar1 commented Mar 10, 2020 •

edited

Loading

bkamins commented Mar 10, 2020

bkamins commented Mar 11, 2020

ssikdar1 commented Mar 12, 2020

bkamins commented Mar 12, 2020

nalimilan left a comment

bkamins commented Mar 12, 2020

	- `coltypes::Bool = true`: Whether to print the column types under column names in data frame.
	- `coltypes::Bool = true`: Whether to print the column types under column names.

	under the column name in the heading. Defaults to true.
	under the column name in the heading. Defaults to `true`.

add displaycoltypes to showrows & show for dataframes #2142

add displaycoltypes to showrows & show for dataframes #2142

Conversation

ssikdar1 commented Mar 5, 2020

Testing

bkamins commented Mar 5, 2020

nalimilan left a comment

Choose a reason for hiding this comment

nalimilan Mar 7, 2020

Choose a reason for hiding this comment

ssikdar1 Mar 8, 2020

Choose a reason for hiding this comment

nalimilan Mar 7, 2020

Choose a reason for hiding this comment

ssikdar1 Mar 8, 2020

Choose a reason for hiding this comment

nalimilan Mar 7, 2020

Choose a reason for hiding this comment

ssikdar1 Mar 8, 2020

Choose a reason for hiding this comment

bkamins commented Mar 7, 2020

ssikdar1 commented Mar 9, 2020

bkamins commented Mar 9, 2020

bkamins commented Mar 9, 2020

ssikdar1 commented Mar 10, 2020

bkamins commented Mar 10, 2020

ssikdar1 commented Mar 10, 2020 • edited Loading

bkamins commented Mar 10, 2020

bkamins commented Mar 11, 2020

ssikdar1 commented Mar 12, 2020

bkamins commented Mar 12, 2020

nalimilan left a comment

Choose a reason for hiding this comment

bkamins commented Mar 12, 2020

ssikdar1 commented Mar 10, 2020 •

edited

Loading