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

Add option to plot classifier leaves as horizontal bars #215

Merged
merged 2 commits into from
Dec 28, 2022

Conversation

mepland
Copy link
Collaborator

@mepland mepland commented Dec 28, 2022

Added leaf_plot_type parameter. Default is pie which replicates prior plots, but now includes a barh option!

dtreeviz

dtreeviz/trees.py Outdated Show resolved Hide resolved
@parrt parrt added the enhancement New feature or request label Dec 28, 2022
@parrt parrt added this to the 2.1 milestone Dec 28, 2022
@parrt parrt changed the base branch from master to dev December 28, 2022 19:35
@parrt
Copy link
Owner

parrt commented Dec 28, 2022

How does the new height look? If you want I can just pull it in and tweak it on my side

@mepland
Copy link
Collaborator Author

mepland commented Dec 28, 2022

dtreeviz

New height looks good!

BTW my next commit should be signed with my ssh key.

@parrt
Copy link
Owner

parrt commented Dec 28, 2022

love it. thanks!

@parrt parrt merged commit 3b30d7d into parrt:dev Dec 28, 2022
@parrt
Copy link
Owner

parrt commented Dec 29, 2022

@tlapusan should we make this bar chart the default leaf? People seem convinced pie charts are bad haha.

@parrt
Copy link
Owner

parrt commented Dec 30, 2022

BTW, it occurs to me guys that we want the leaves to show relative area and not overlapping. "stacked" barh is what we want I think but we'd want this non-overlapping regions. E.g., if two classes are 10 and 10 samples each, then we don't want to hide one. We want equal visual regions. @tlapusan

@mepland
Copy link
Collaborator Author

mepland commented Dec 30, 2022

@parrt They don't overlap, due to the left param here:

data_cum = 0
for i in range(len(counts)):
    width = counts[i]
    plt.barh(0, width, left=data_cum, color=colors[i], height=1, edgecolor=graph_colors['rect_edge'], linewidth=0.5)
    data_cum += width

Also I originally had the length of the bar plots scale with the total number of samples in the leaf, but we turned that off; though I wouldn't mind bringing it back - maybe as an option:
Screenshot 2022-11-12 at 13 29 12

@parrt
Copy link
Owner

parrt commented Dec 30, 2022

cool. yeah, i had the pie size scale a bit. I think maybe scaling with n is good. @tlapusan?

Also we should remove the 0..57 x-axis labels; i think n=58 with the y class is best. actually, don't even need the class. the color says it. maybe this is an option. can't remember.

@mepland
Copy link
Collaborator Author

mepland commented Dec 30, 2022

Yeah we updated it to not have x-axis ticks and just put the n=... below the bar. This is just the only old screenshot I have where the size changed with n.

@parrt
Copy link
Owner

parrt commented Dec 30, 2022

ah, cool :) yeah, i think it looks good:

Screenshot 2022-12-30 at 2 30 42 PM

@parrt
Copy link
Owner

parrt commented Dec 30, 2022

@mepland be advised that I did a rename of the argument: 2004e8e

@mepland
Copy link
Collaborator Author

mepland commented Dec 31, 2022

ah, cool :) yeah, i think it looks good:

Screenshot 2022-12-30 at 2 30 42 PM

@parrt did you change & commit the code to re-enable horizontal scaling with n? This looks great, I just don't see the change in code anywhere.

Also, I'm fine if this is the default behavior, but @tlapusan wanted them to all be the same size. Maybe we should make an option to control this behavior?

@parrt
Copy link
Owner

parrt commented Dec 31, 2022

Latest dev branch shows these two:

viz_model.view()
viz_model.view(leaftype='barh')

image
image

I think it's important to show where the mass of samples go in the leaves but I can imagine making this an option, thought it mirrors the current behavior with size now.

@parrt
Copy link
Owner

parrt commented Dec 31, 2022

I also think I'm going to make the bar charts the default, since most people have an immediate "but but but I was trying to the pie charts are bad" reaction.

@parrt
Copy link
Owner

parrt commented Dec 31, 2022

See: 423060f

@tlapusan
Copy link
Collaborator

tlapusan commented Jan 1, 2023

@tlapusan should we make this bar chart the default leaf? People seem convinced pie charts are bad haha.

hmm, personally I like the pie charts :) highlights better the leaves and make the viz a little more interesting.
@parrt curious why the people think that the pies are a bad option. If I think about it...pies are very popular in general when somebody wants to visually represent the categories from a population.

If you heard this feedback many times, I'm ok to choose the barh as the default.

@tlapusan
Copy link
Collaborator

tlapusan commented Jan 1, 2023

ah, cool :) yeah, i think it looks good:
Screenshot 2022-12-30 at 2 30 42 PM

@parrt did you change & commit the code to re-enable horizontal scaling with n? This looks great, I just don't see the change in code anywhere.

Also, I'm fine if this is the default behavior, but @tlapusan wanted them to all be the same size. Maybe we should make an option to control this behavior?

@mepland I'm also for making the leaf size based on its input samples. Don't remember when I mentioned that I would like them to be the same size. Sorry if I did :)

@mepland
Copy link
Collaborator Author

mepland commented Jan 1, 2023

@mepland I'm also for making the leaf size based on its input samples. Don't remember when I mentioned that I would like them to be the same size. Sorry if I did :)

No problem, I may have miss-understood.

@parrt
Copy link
Owner

parrt commented Jan 1, 2023

No problem. We can always back this out if @tlapusan has a good reason to fix the size. :) Oh, missed his comment above. Well all of the fancy pants people are taught that pie charts are bad. In fact a well-known guy using our software said he loves the software except for the pie chart. I like to pie charts myself. Maybe it's a good idea to switch it back to pie. ok, i'll switch back.

parrt added a commit that referenced this pull request Jan 1, 2023
Signed-off-by: Terence Parr <parrt@antlr.org>
@mepland mepland deleted the bar_leaf_plot branch January 14, 2023 23:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants