remove dependency on CategoricalArrays.jl in legacy show #2427

bkamins · 2020-09-12T22:18:57Z

Using reflection is the best what could come up with unless there is a solution to https://stackoverflow.com/questions/63860020/how-to-get-a-list-of-modules-imported-by-a-given-module (which seems unlikely - I have checked the source code, and it seems that this part of the functionality is not exposed to Julia code and is only available in C).

The approach is assuming that if something is pretending to be CategoricalArrays.jl we can just treat it in display as if it were CategoricalArrays.jl (and in general such pretending is unlikely).

quinnj · 2020-09-13T03:23:11Z

src/abstractdataframe/show.jl

+    # the T is an instance of CategoricalValue from CategoricalArrays.jl
+    if hasproperty(T, :name) &&
+        nameof(T.name.module) === :CategoricalArrays &&
+        T.name.name === :CategoricalValue


Do we not care if the T.parameters[1] <: AbstractString here?

bacause we are replacing the behaviour for CategoricalValue{<:AbstractString} defined earlier in line https://github.com/JuliaData/DataFrames.jl/pull/2427/files#diff-38a6c4f0cb7743ccc6d4e8470ebdbd11L69. I just want to ensure I am matching functionality 1-2-1 exactly.

quinnj

LGTM; hacky, but whatever, it does the job. The alternative route is we put something in DataAPI.jl like iscategorical(x) or something that CategoricalArrays could overload.

bkamins · 2020-09-13T06:12:09Z

The alternative route is we put something in DataAPI.jl like iscategorical(x) or something that CategoricalArrays could overload.

I was also thinking about it, but I think it is best for @nalimilan to decide what is best. In particular if we have iscategorical in DataAPI.jl we probably should handle both: CategoricalValue (for use cases like here) and CategoricalArray (to allow e.g. using it in /src/dataframerow/utils.jl to fire a fast path for CategoricalVector without having to change the code much - AFAICT @nalimilan is working on it now).

nalimilan · 2020-09-13T14:07:30Z

Mmm, this is definitely very hacky... I see two other possibly clearner solutions:

use an IOContext property that CategoricalValue would dected
use print everywhere

I wonder what the drawbacks of using print would be. Besides omitted quotes when printing string and : when printing symbols, it also represents Date("2020-09-13") as 2020-09-13 and Day(1) as 1 day. Maybe that's what we're actually looking for? It's not clear why we do something special for strings but not for other types. What do you think?

bkamins · 2020-09-13T16:33:12Z

If we are at it I would actually prefer to switch to print if everyone is OK with this (I thought there was some reason to use show but maybe there was none). PrettyTables.jl uses print.

If we use print we do not need to be hacky here.

bkamins · 2020-09-13T19:32:50Z

OK - I have switched show to print. I think we are OK to do this as we show eltype in the header (so e.g. not escaping Char is OK as you see in the changed tests).

@rofinn - could you please have a look at it as you use print in PrettyTables.jl

bkamins · 2020-09-13T20:24:24Z

CC @ronisbr not @rofinn - sorry for a typo.

bkamins · 2020-09-14T08:48:40Z

TODO. Check what is printed with this PR if you call:

julia> DataFrame(x=[true,false], y=[0x1, 0x2])
2×2 DataFrame
│ Row │ x    │ y     │
│     │ Bool │ UInt8 │
├─────┼──────┼───────┤
│ 1   │ 1    │ 0x01  │
│ 2   │ 0    │ 0x02  │

(this is the output from legacy code)

matthieugomez · 2020-10-05T17:33:52Z

Stata prints strings in a dataset without quotes but, for clarity, it also uses a different color (red). Maybe that would be useful to do the same.

bkamins · 2020-10-05T19:15:26Z

CC @ronisbr, as this PR will not get merged. #2429 is the way to go, and when that is merged I will close this. I keep it open only to make sure #2429 correctly handles CategoricalArrays.jl without introducing a dependency.

bkamins · 2020-10-11T17:11:20Z

@nalimilan: given the discussion with @ronisbr we should merge this PR anyway (we need this to get rid of CategoricalArrays.jl dependency for HTML and LaTeX even if we start using PrettyTables.jl for text/plain)

nalimilan · 2020-10-12T20:24:12Z

src/abstractdataframe/show.jl

-    if T <: CategoricalValue
+    # This is only type display shortening so we
+    # are OK with any T whose name starts with Categorical here
+    if startswith(sT, "Categorical")


We only need to handle CategoricalValue, right. so maybe better strictly check for "CategoricalValue"? Abbreviating other types could be weird.

OK - I have written it when CategoricalString was present.

src/abstractdataframe/show.jl

nalimilan · 2020-10-12T20:30:25Z

src/abstractdataframe/show.jl

 `truncstring` indicates the approximate number of text characters width to truncate
 the output (if it is a non-positive value then no truncation is applied).
 """
-function ourshow(io::IO, x::Any, truncstring::Int; styled::Bool=false)
+function ourshow(io::IO, x::T, truncstring::Int; styled::Bool=false) where T


T isn't used right?

ah - yes; it was earlier, but we ended up not using it. Fixing.

src/abstractdataframe/show.jl

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>

bkamins · 2020-10-13T11:18:43Z

Now I remember why I used "Categorical" not "CategoricalValue" 😄. It is because CategoricalArrays.jl might not be in scope in which case CategoricalArray.CategoricalValue is eltype name. I have added || now to handle this case.

src/abstractdataframe/show.jl

bkamins · 2020-10-13T15:41:10Z

Thank you!

remove dependency on CategoricalArrays.jl in legacy show

c75ced2

bkamins requested a review from nalimilan September 12, 2020 22:19

bkamins added ecosystem Issues in DataFrames.jl ecosystem non-breaking The proposed change is not breaking labels Sep 12, 2020

quinnj reviewed Sep 13, 2020

View reviewed changes

quinnj approved these changes Sep 13, 2020

View reviewed changes

switch show to print

5a74e97

nalimilan mentioned this pull request Sep 16, 2020

[BREAKING] Add PrettyTables.jl backend for printing DataFrames #2429

Merged

fix Char on Julia 1.0

409b8b4

nalimilan reviewed Oct 12, 2020

View reviewed changes

bkamins commented Oct 12, 2020

View reviewed changes

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

Update src/abstractdataframe/show.jl

da7cab1

nalimilan reviewed Oct 12, 2020

View reviewed changes

bkamins commented Oct 12, 2020

View reviewed changes

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

Update src/abstractdataframe/show.jl

2cf6014

nalimilan reviewed Oct 13, 2020

View reviewed changes

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

bkamins and others added 2 commits October 13, 2020 13:09

Update src/abstractdataframe/show.jl

8b40007

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>

fix case when CategoricalArrays is not in scope

25915a6

nalimilan reviewed Oct 13, 2020

View reviewed changes

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

Update src/abstractdataframe/show.jl

e2048f7

nalimilan approved these changes Oct 13, 2020

View reviewed changes

bkamins merged commit f507944 into JuliaData:master Oct 13, 2020

bkamins deleted the categorical_show branch October 13, 2020 15:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove dependency on CategoricalArrays.jl in legacy show #2427

remove dependency on CategoricalArrays.jl in legacy show #2427

bkamins commented Sep 12, 2020

quinnj Sep 13, 2020

bkamins Sep 13, 2020

quinnj left a comment

bkamins commented Sep 13, 2020

nalimilan commented Sep 13, 2020

bkamins commented Sep 13, 2020

bkamins commented Sep 13, 2020

bkamins commented Sep 13, 2020

bkamins commented Sep 14, 2020

matthieugomez commented Oct 5, 2020 •

edited

Loading

bkamins commented Oct 5, 2020

bkamins commented Oct 11, 2020

nalimilan Oct 12, 2020

bkamins Oct 12, 2020

nalimilan Oct 12, 2020

bkamins Oct 12, 2020

bkamins commented Oct 13, 2020

bkamins commented Oct 13, 2020

remove dependency on CategoricalArrays.jl in legacy show #2427

remove dependency on CategoricalArrays.jl in legacy show #2427

Conversation

bkamins commented Sep 12, 2020

quinnj Sep 13, 2020

Choose a reason for hiding this comment

bkamins Sep 13, 2020

Choose a reason for hiding this comment

quinnj left a comment

Choose a reason for hiding this comment

bkamins commented Sep 13, 2020

nalimilan commented Sep 13, 2020

bkamins commented Sep 13, 2020

bkamins commented Sep 13, 2020

bkamins commented Sep 13, 2020

bkamins commented Sep 14, 2020

matthieugomez commented Oct 5, 2020 • edited Loading

bkamins commented Oct 5, 2020

bkamins commented Oct 11, 2020

nalimilan Oct 12, 2020

Choose a reason for hiding this comment

bkamins Oct 12, 2020

Choose a reason for hiding this comment

nalimilan Oct 12, 2020

Choose a reason for hiding this comment

bkamins Oct 12, 2020

Choose a reason for hiding this comment

bkamins commented Oct 13, 2020

bkamins commented Oct 13, 2020

matthieugomez commented Oct 5, 2020 •

edited

Loading