Skip to content

Add a text for displaying values on histogram.#1496

Merged
rcap107 merged 12 commits into
skrub-data:mainfrom
lionelkusch:PR_showing_number_bar_plot
Oct 17, 2025
Merged

Add a text for displaying values on histogram.#1496
rcap107 merged 12 commits into
skrub-data:mainfrom
lionelkusch:PR_showing_number_bar_plot

Conversation

@lionelkusch

Copy link
Copy Markdown
Contributor

For the histogram, I add text for displaying the percentage of each bar, the same as for values_count. I try to reuse the idea of display value for displaying on the histogram.

This pull request was made for fixing the issue #1454.

For the histogramm, I add text for displaying
the percentage of each bar, same as for values_count.
I try to reuse the ideas of display value for displaying
on the histogram
[doc build]
@Vincent-Maladiere

Copy link
Copy Markdown
Member

Hey @lionelkusch thank you for this PR! #1454 is kinda tricky and we're not sure how to move forward.

Your suggestion is interesting, but my understanding is that we'd rather have the height of each vertical bar rather than the percentile of their distribution. Additionally, we'd also like to have the total count for the categorical distribution (displayed horizontally). We need to see whether adding these figures would take too much space to be useful or not.

We can use this PR to explore different options.

@lionelkusch

Copy link
Copy Markdown
Contributor Author

This PR is quite limited to add information to the histogram is the same spirit as for the value_counted.
I pushed further in PR #1506 . For adding the height of the bar.
Tell me what final form you want.

@lionelkusch

Copy link
Copy Markdown
Contributor Author

Addressing your remarks:

The problem when you combine the two in the same graphic is that the text will be longer than the columns. This won't look nice and it will be difficult to automatically add the text to the graphic.

If the text crossing the bar edge doesn't look good, we can choose to always have the text outside of the bar, increasing the figure size slightly if necessary.

Adding the number of elements creates some variability and creates a difficult problem for the position of the text relative to the bar.

I think this can be addressed by controlling the number of digits in the format using custom rules, like full representation when the entries are smaller than 10,000 and scientific notation otherwise. We would have an upper bound on the size of the text.


What do the others think?

I agree on all points here and I would also prefer having both numbers visible at the same time.

Adding the number of elements creates some variability and creates a difficult problem for the position of the text relative to the bar.

How about adding the % to the right (the top) of the plot, aligned with each bar? With a fixed format (like 3 significant digits) the space should remain mostly constant, while the actual value is still represented in the same way as it is now. If the % sign takes too much space, it could be added above the column of numbers as an annotation.

Originally posted by @rcap107 in #1506 (comment)

I put back the comments of the PR #1506 about the graphic representation.

@rcap107

rcap107 commented Aug 28, 2025

Copy link
Copy Markdown
Member

I quite like the current version, thanks a lot @lionelkusch!

I just realized is that we might want to remove the % numbers for numerical features, because bins for numerical values are defined somewhat arbitrarily, so the % numbers for them are not really meaningful.

@rcap107

rcap107 commented Sep 24, 2025

Copy link
Copy Markdown
Member

Hey @lionelkusch, do you think you finish this PR? As we discussed, the current version is good, the only change would be removing the % from the numerical columns

I hope there won't be too many conflicts to deal with

@lionelkusch

Copy link
Copy Markdown
Contributor Author

@rcap107 I am sorry, I forgot to do the modification.
I did it. I think that the PR is read.

@rcap107

rcap107 commented Sep 24, 2025

Copy link
Copy Markdown
Member

thanks a lot! I'm wondering if the plots would look better without the right and top spines, like the current version 🤔

Screenshot_20250924_131132 Screenshot_20250924_131208

@rcap107 rcap107 added this to the 0.7.0 milestone Sep 24, 2025
@lionelkusch

Copy link
Copy Markdown
Contributor Author

I propose you this solution:
image

@GaelVaroquaux

Copy link
Copy Markdown
Member

Is there a place where I can look at a full HTML version, to interact a bit with it?

@rcap107

rcap107 commented Sep 24, 2025

Copy link
Copy Markdown
Member

Is there a place where I can look at a full HTML version, to interact a bit with it?

I was comparing the table report in the homepage of the generated docs with that in the live version

@rcap107

rcap107 commented Sep 24, 2025

Copy link
Copy Markdown
Member

I propose you this solution: image

I think it's ok, I was worried that the tick might be read by someone as a "minus" or "negative", but it doesn't really make sense for percentages

@lionelkusch

Copy link
Copy Markdown
Contributor Author

I removed the tick line; I think it's not needed it for the readability of the figure.

@GaelVaroquaux

Copy link
Copy Markdown
Member

It's quite OK, actually:

image

My concern is: information overload. The more we put, the harder it is for a user to focus and to get the message. It brings us to the gold rule of communication: less in more.

But you've managed to keep it really light.

An ideal option, would have been to have a "hover": information that appears when the mouse goes over the relevant graph. But it's hard to do with our current toolkit.

I'm +1 for merge on this. Congratulations!!

@rcap107 rcap107 linked an issue Oct 10, 2025 that may be closed by this pull request
@rcap107

rcap107 commented Oct 16, 2025

Copy link
Copy Markdown
Member

Hey @lionelkusch, I keep on losing track of this issue 🙈 my bad

We can merge after removing the barplot labels for the numerical features, like here
image

The bins are decided arbitrarily, so the size of each bin is not particularly informative.

(I thought I had already mentioned this here, but I guess I forgot)

@lionelkusch

Copy link
Copy Markdown
Contributor Author

Are you sure that you want to remove the label?
I ask the question because it seems to me that this was the starting point of the PR, as described in the issue #1454.

@rcap107

rcap107 commented Oct 16, 2025

Copy link
Copy Markdown
Member

Are you sure that you want to remove the label? I ask the question because it seems to me that this was the starting point of the PR, as described in the issue #1454.

#1454 mentioned both categorical and numerical features, but given that the distribution of numerical features is generated with "best effort" bins, the actual value of the bins isn't very meaningful, so I think it can be removed.

For categorical features, each category is well delimited so it makes sense to have both the number of occurrences and the %, so the current version is good and should remain as you wrote it.

@lionelkusch

Copy link
Copy Markdown
Contributor Author

done

@rcap107 rcap107 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good to me, thanks a lot @lionelkusch. It was a bit convoluted (my fault for that), but the result is a nice addition to the TableReport.

@rcap107 rcap107 merged commit 6e03d24 into skrub-data:main Oct 17, 2025
29 checks passed
@MA-inria

Copy link
Copy Markdown

Thank you all for merging this PR

dierickxsimon pushed a commit to dierickxsimon/skrub that referenced this pull request Oct 23, 2025
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.

Showing values for bar plots in TableReport

5 participants