fix print_tree and add test #42

jonalm · 2020-01-09T20:59:01Z

fix behaviour of maxdepth argument to print_tree

codecov-io · 2020-01-09T21:08:26Z

Codecov Report

❗ No coverage uploaded for pull request base (master@203efb8). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master      #42   +/-   ##
=========================================
  Coverage          ?   57.64%           
=========================================
  Files             ?        3           
  Lines             ?      314           
  Branches          ?        0           
=========================================
  Hits              ?      181           
  Misses            ?      133           
  Partials          ?        0

Impacted Files	Coverage Δ
src/AbstractTrees.jl	`54.19% <ø> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 203efb8...6a896a3. Read the comment docs.

jonalm · 2020-01-09T21:29:18Z

Example 1

struct SingleChildInfiniteDepth end
AbstractTrees.children(::SingleChildInfiniteDepth) = (SingleChildInfiniteDepth(),)
print_tree(SingleChildInfiniteDepth(), 5)

prints

SingleChildInfiniteDepth()
└─ SingleChildInfiniteDepth()
   └─ SingleChildInfiniteDepth()
      └─ SingleChildInfiniteDepth()
         └─ SingleChildInfiniteDepth()
            └─ SingleChildInfiniteDepth()

Example 2

struct Num{I} end
Num(I::Int) = Num{I}()
Base.show(io::IO, ::Num{I}) where {I} = print(io, I)
AbstractTrees.children(::Num{I}) where {I} = (Num(I+1),Num(I+1))
print_tree(Num(0),3)

prints

0
├─ 1
│  ├─ 2
│  │  ├─ 3
│  │  └─ 3
│  └─ 2
│     ├─ 3
│     └─ 3
└─ 1
   ├─ 2
   │  ├─ 3
   │  └─ 3
   └─ 2
      ├─ 3
      └─ 3

timholy · 2020-01-10T09:46:43Z

Since this has a test I'm inclined to go with this version. Should we consider adding a line (Tree was truncated at depth $maxdepth) at the end in cases where the whole tree wasn't printed? Or would we find that annoying?

jonalm · 2020-01-10T10:08:24Z

I have use cases of the 'print_tree' function where it creates a text summary report about a dataset, whose underlying tree structure may be confusing for the person who reads the report. I would be slightly annoyed if I had to remove the "tree truncated" line if I don't want it in the output.

maxdepth is a user input so it should be clear for the user (could be made a named variable to make it even more clear). The only issue I see is the default value, which was 5 in the original code. We could set it to Inf, or remove the default value, to force the user to make an explicit choice.

timholy · 2020-01-10T10:25:06Z

Totally understood re the annoyance.

This is a bit tricky. Julia truncates arrays and other containers when they get too big; in general there seems to be a pretty strong commitment to avoid "endless printing." So following that model seems useful here. But then I think we need some visual indicator of truncation (e.g., arrays use ellipses), at least in the case where the user hasn't explicitly provided maxdepth, otherwise s/he might not know there is more to the tree than what is shown.

What if we change the default value for maxdepth to nothing, and then in the code

print_warning_if_trunc = false
if maxdepth === nothing
    maxdepth = 5
    print_warning_if_trunc = true
end

Ellipses are of course an alternative to printing a warning at the end. I'm just keying off your comment #38 (comment), plus the fact that if you've explicitly asked for a certain depth you might want to omit the ellipses.

jonalm · 2020-01-10T12:29:44Z

This was more tricky than I anticipated, as the print_tree is recursive. I couldn't manage to print a single final warning which only triggered if the tree depth was deeper than maxdepth.

Consider therefore this implementation which indicate truncation with a single vertical ellipsis under final node if maxdepth isn't set (and defaults to 5) and the tree has a depth which is more that 5. If maxdepth is specified then print_tree does not indicate the truncation, in accordance with the above discussion.

Ex1

ellipsis if default maxdepth and depth > maxdepth

struct Num{I} end
Num(I::Int) = Num{I}()
Base.show(io::IO, ::Num{I}) where {I} = print(io, I)
AbstractTrees.children(::Num{I}) where {I} = (Num(I+1),Num(I+1))
print_tree(Num(0) )

prints

0
├─ 1
│  ├─ 2
│  │  ├─ 3
│  │  │  ├─ 4
│  │  │  │  ├─ 5
│  │  │  │  │  ⋮
│  │  │  │  │  
│  │  │  │  └─ 5
│  │  │  │     ⋮
│  │  │  │     
│  │  │  └─ 4
│  │  │     ├─ 5
│  │  │     │  ⋮
│  │  │     │  
│  │  │     └─ 5
│  │  │        ⋮
│  │  │        
│  │  └─ 3
│  │     ├─ 4
│  │     │  ├─ 5
│  │     │  │  ⋮
│  │     │  │  
│  │     │  └─ 5
│  │     │     ⋮
│  │     │     
│  │     └─ 4
│  │        ├─ 5
│  │        │  ⋮
│  │        │  
│  │        └─ 5
│  │           ⋮
│  │           
│  └─ 2
│     ├─ 3
│     │  ├─ 4
│     │  │  ├─ 5
│     │  │  │  ⋮
│     │  │  │  
│     │  │  └─ 5
│     │  │     ⋮
│     │  │     
│     │  └─ 4
│     │     ├─ 5
│     │     │  ⋮
│     │     │  
│     │     └─ 5
│     │        ⋮
│     │        
│     └─ 3
│        ├─ 4
│        │  ├─ 5
│        │  │  ⋮
│        │  │  
│        │  └─ 5
│        │     ⋮
│        │     
│        └─ 4
│           ├─ 5
│           │  ⋮
│           │  
│           └─ 5
│              ⋮
│              
└─ 1
   ├─ 2
   │  ├─ 3
   │  │  ├─ 4
   │  │  │  ├─ 5
   │  │  │  │  ⋮
   │  │  │  │  
   │  │  │  └─ 5
   │  │  │     ⋮
   │  │  │     
   │  │  └─ 4
   │  │     ├─ 5
   │  │     │  ⋮
   │  │     │  
   │  │     └─ 5
   │  │        ⋮
   │  │        
   │  └─ 3
   │     ├─ 4
   │     │  ├─ 5
   │     │  │  ⋮
   │     │  │  
   │     │  └─ 5
   │     │     ⋮
   │     │     
   │     └─ 4
   │        ├─ 5
   │        │  ⋮
   │        │  
   │        └─ 5
   │           ⋮
   │           
   └─ 2
      ├─ 3
      │  ├─ 4
      │  │  ├─ 5
      │  │  │  ⋮
      │  │  │  
      │  │  └─ 5
      │  │     ⋮
      │  │     
      │  └─ 4
      │     ├─ 5
      │     │  ⋮
      │     │  
      │     └─ 5
      │        ⋮
      │        
      └─ 3
         ├─ 4
         │  ├─ 5
         │  │  ⋮
         │  │  
         │  └─ 5
         │     ⋮
         │     
         └─ 4
            ├─ 5
            │  ⋮
            │  
            └─ 5
               ⋮

Ex2

no ellipsis if default maxdepth and depth < maxdepth

struct Num{I} end
Num(I::Int) = Num{I}()
Base.show(io::IO, ::Num{I}) where {I} = print(io, I)
AbstractTrees.children(::Num{I}) where {I} = (Num(I+1),Num(I+1))
AbstractTrees.children(::Num{3}) = ()
print_tree(Num(0))

0
├─ 1
│  ├─ 2
│  │  ├─ 3
│  │  └─ 3
│  └─ 2
│     ├─ 3
│     └─ 3
└─ 1
   ├─ 2
   │  ├─ 3
   │  └─ 3
   └─ 2
      ├─ 3
      └─ 3

Ex3

no ellipsis if maxdepth set

struct SingleChildInfiniteDepth end
AbstractTrees.children(::SingleChildInfiniteDepth) = (SingleChildInfiniteDepth(),)
print_tree(SingleChildInfiniteDepth(),6)

prints

SingleChildInfiniteDepth()
└─ SingleChildInfiniteDepth()
   └─ SingleChildInfiniteDepth()
      └─ SingleChildInfiniteDepth()
         └─ SingleChildInfiniteDepth()
            └─ SingleChildInfiniteDepth()
               └─ SingleChildInfiniteDepth()

@timholy @ericphanson if you agree to this, I can add some more test which covers the above mentioned cases.

timholy · 2020-01-10T12:43:38Z

I'm extremely happy with this!

I suspect you could solve the recursive problem by adding a keyword argument topcall=true and then having _print_tree call itself with topcall=false. But like I said, I'm very happy with the solution you came up with, so I am not urging you to change it.

Tests of your improved behavior would indeed be nice.

jonalm · 2020-01-10T12:52:44Z

Good to hear. I'll await @ericphanson comments, as I'm hijacking his pr.

Regarding the topcall approach. For the sake of my understanding: how do you determine whether the tree is deeper that maxdepth or not? I.e. how to distinguish between Ex1 and Ex2 in my examples above, without having a callback or a global variable?

timholy · 2020-01-10T13:40:28Z

One possible way: if _print_tree could return true if it truncated. Then a truncated |= _print_tree(args...) recursive call would let you figure out if anything truncated. (_print_tree currently calls print_tree rather than itself, but that could be changed.) Then print_tree could discard the truncated output.

ericphanson · 2020-01-12T15:52:21Z

Thanks for pinging me on this :).

I think this solution would not be optimal for my use case, namely using print_tree for a show method for Convex.jl objects, implemented mostly in Convex#325. The reason for that is we provide a global Convex.MAXDEPTH which can be set by the user to customize printing, and hence we may be frequently using a non-default maxdepth argument but still want ellipses. I think users would be surprised/confused if they e.g. change a constraint in their model but see no change or ellipses in the printing.

However, we've already vendored print_tree in Convex.jl, so I think it's not a big deal if the methods diverge; after all, we probably are using it for a different use case than many users, so maybe it really should be a different method.

We may have another reason to stick to a vendored copy of the function, which is that I sometimes saw somewhat strange errors when trying out variants of Convex#325 locally, I think which may be related to #15. That makes me think it's probably safer for us to keep the function vendored, because I fear our show methods may be fragile to implementation details of print_tree (or things that look like implementation details but are actually breaking; such breakage could probably be fixed with a simple PR, but it seems like a bad story for users to have fragile show methods that depend on an external package).

So in all, such a change wouldn't support my the case I created the other PR for, but that's okay and we can just keep our vendored copy.

I do personally have a preference for always printing ellipses if something is truncated because I think it helps avoid confusion. One idea is ellipses=true keyword argument to control it explicitly instead of having it implicit in whether or not maxdepth is passed.

jonalm · 2020-01-12T23:40:44Z

I've incorporated a couple of tests for the new implementation of the different cases.

@timholy

One possible way: if _print_tree could return true if it truncated. Then a truncated |= _print_tree(args...) recursive call would let you figure out if anything truncated. (_print_tree currently calls print_tree rather than itself, but that could be changed.) Then print_tree could discard the truncated output.

The return value approach sounds obvious in hindsight. Thanks.

@ericphanson

So in all, such a change wouldn't support my the case I created the other PR for, but that's okay and we can just keep our vendored copy.

Great!

@ericphanson

I do personally have a preference for always printing ellipses if something is truncated because I think it helps avoid confusion. One idea is ellipses=true keyword argument to control it explicitly instead of having it implicit in whether or not maxdepth is passed.

In the current implementation you can force printing of ellispes by indicate_truncation=true.
I guess this boils down to differences in personal preference for its default value. I have a (minor) preference for it to be false by default if maxdepth is explicitly set, but I'm fine with changing it if people here want it otherwise. I'm not really sure how decisions are being made in the JuliaCollections org.

jonalm · 2020-01-12T23:42:54Z

It seems like I broke the build on Julia 0.7 build. ~~Any idea what that is about?~~

Test docstrings apparently, and out of sync with master runtests.jl

jonalm · 2020-01-13T22:38:13Z

Have fixed some tests and added to the documentation string. I changed my mind on indicate_truncation default value (is now true), as it made the documentation much clearer and easier to write.

timholy

This is lovely and basically ready to merge. One small comment re documentation or behavior.

timholy · 2020-01-14T10:01:58Z

src/AbstractTrees.jl

-value can be overriden by the `maxdepth` parameter. The charset to use in
+value can be overriden by the `maxdepth` parameter. Nodes that are truncated are
+indicated by a vertical ellipsis below the truncated node, this indication can be
+turned off by providing `indicate_truncation=false` as a kwarg. The charset to use in


With the current implementation you have to provide a non-default value for maxdepth too:

julia> print_tree(SingleChildInfiniteDepth(); indicate_truncation=false) SingleChildInfiniteDepth() └─ SingleChildInfiniteDepth() └─ SingleChildInfiniteDepth() └─ SingleChildInfiniteDepth() └─ SingleChildInfiniteDepth() └─ SingleChildInfiniteDepth() ⋮

but

julia> print_tree(SingleChildInfiniteDepth(), 5; indicate_truncation=false) SingleChildInfiniteDepth() └─ SingleChildInfiniteDepth() └─ SingleChildInfiniteDepth() └─ SingleChildInfiniteDepth() └─ SingleChildInfiniteDepth() └─ SingleChildInfiniteDepth()

Worth mentioning this? Or should we change the behavior?

Good catch. We should change the behavior, and I just did. indicate_truncation should now work as expected, regardless of maxdepth.

I also removed the whole maxdepth=nothing business as it is no longer needed.

timholy · 2020-01-14T11:26:27Z

Thanks for a great contribution! New release coming...

codecov-commenter · 2024-12-08T07:08:44Z

Codecov Report

Attention: Patch coverage is 94.73684% with 1 line in your changes missing coverage. Please review.

Project coverage is 57.64%. Comparing base (0e2ae9d) to head (6a896a3).
Report is 211 commits behind head on master.

Files with missing lines	Patch %	Lines
src/AbstractTrees.jl	94.73%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #42      +/-   ##
==========================================
+ Coverage   57.00%   57.64%   +0.64%     
==========================================
  Files           3        3              
  Lines         307      314       +7     
==========================================
+ Hits          175      181       +6     
- Misses        132      133       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jonalm added 3 commits January 9, 2020 21:45

fix print_tree and add test

54d2726

add comment to test

67139c8

remove commented code

50f452a

jonalm mentioned this pull request Jan 9, 2020

Add ellipsis char; respect maxdepth for printing #38

Closed

improve comment

807262d

add truncation indicator

de39c9d

jonalm added 3 commits January 12, 2020 23:32

add updated tests

6c64b8d

Merge branch 'master' into fix-maxdepth-in-print_tree

2f77269

add indicate_truncation as explisit keyword

f1867db

jonalm added 4 commits January 13, 2020 22:54

merge master

203efb8

fix test comments

6663586

fix push reject

5502df7

change default indicate_truncation, updattte docs

2ff3bc5

jonalm added 2 commits January 13, 2020 23:57

make tests compatible with julia 1.0

1ab24c6

minor doc issue

e05419a

timholy reviewed Jan 14, 2020

View reviewed changes

simplify code

6a896a3

timholy merged commit 784504f into JuliaCollections:master Jan 14, 2020

jlumpe mentioned this pull request Feb 10, 2020

print_tree does not respect maxdepth #37

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix print_tree and add test #42

fix print_tree and add test #42

jonalm commented Jan 9, 2020

codecov-io commented Jan 9, 2020 •

edited

Loading

jonalm commented Jan 9, 2020

timholy commented Jan 10, 2020

jonalm commented Jan 10, 2020

timholy commented Jan 10, 2020 •

edited

Loading

jonalm commented Jan 10, 2020 •

edited

Loading

timholy commented Jan 10, 2020

jonalm commented Jan 10, 2020

timholy commented Jan 10, 2020

ericphanson commented Jan 12, 2020

jonalm commented Jan 12, 2020

jonalm commented Jan 12, 2020 •

edited

Loading

jonalm commented Jan 13, 2020 •

edited

Loading

timholy left a comment

timholy Jan 14, 2020

jonalm Jan 14, 2020

jonalm Jan 14, 2020

timholy commented Jan 14, 2020

codecov-commenter commented Dec 8, 2024

fix print_tree and add test #42

fix print_tree and add test #42

Conversation

jonalm commented Jan 9, 2020

codecov-io commented Jan 9, 2020 • edited Loading

Codecov Report

jonalm commented Jan 9, 2020

Example 1

Example 2

timholy commented Jan 10, 2020

jonalm commented Jan 10, 2020

timholy commented Jan 10, 2020 • edited Loading

jonalm commented Jan 10, 2020 • edited Loading

Ex1

Ex2

Ex3

timholy commented Jan 10, 2020

jonalm commented Jan 10, 2020

timholy commented Jan 10, 2020

ericphanson commented Jan 12, 2020

jonalm commented Jan 12, 2020

jonalm commented Jan 12, 2020 • edited Loading

jonalm commented Jan 13, 2020 • edited Loading

timholy left a comment

Choose a reason for hiding this comment

timholy Jan 14, 2020

Choose a reason for hiding this comment

jonalm Jan 14, 2020

Choose a reason for hiding this comment

jonalm Jan 14, 2020

Choose a reason for hiding this comment

timholy commented Jan 14, 2020

codecov-commenter commented Dec 8, 2024

Codecov Report

codecov-io commented Jan 9, 2020 •

edited

Loading

timholy commented Jan 10, 2020 •

edited

Loading

jonalm commented Jan 10, 2020 •

edited

Loading

jonalm commented Jan 12, 2020 •

edited

Loading

jonalm commented Jan 13, 2020 •

edited

Loading