Skip to content

Add a check box in the distribution of TableReport to switch between a percentage of numbers in the plot.#1506

Closed
lionelkusch wants to merge 16 commits into
skrub-data:mainfrom
lionelkusch:PR_test_figure_html
Closed

Add a check box in the distribution of TableReport to switch between a percentage of numbers in the plot.#1506
lionelkusch wants to merge 16 commits into
skrub-data:mainfrom
lionelkusch:PR_test_figure_html

Conversation

@lionelkusch

Copy link
Copy Markdown
Contributor

This PR is based on the PR #1496.

This PR solves the desire requirement behaviour of the issue #1454 .

The principle of this feature is to use JavaScript to send a message which changes the style of the html division of the figure between 'none' and 'block' (default value). This means that there are 2 figures created for each column: one with a percentage label and one with normal numbers.

WARNING: The percentage is fixed in the size of the display number, which is not the case for the number. This can create some not rendering well for large numbers.

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]
@jeromedockes

Copy link
Copy Markdown
Member

thanks a lot @lionelkusch this is neat! However I wonder if it is worth doubling to computation time used to produce a report as this is something we've been meaning to keep fast for interactive use in a notebook. and there is the added complexity to the html and javascript 🤔 What do others think?

@GaelVaroquaux

Copy link
Copy Markdown
Member

I have the same question as @jeromedockes (sorry @lionelkusch , we really appreciate your involvement)

@lionelkusch

Copy link
Copy Markdown
Contributor Author

In the short time of the sprint, I want it to propose something functional. The creation of two independent figures was the simplest solution for me.
However, if you want to include this new feature, I can provide a new version more optimise, which generates two versions of the same figure with different labels. This will increase a few milliseconds the time of computation.

@Vincent-Maladiere

Copy link
Copy Markdown
Member

What about a simpler "abs value (percentile)" kind of display, for each bar?

@Vincent-Maladiere

Copy link
Copy Markdown
Member

@lionelkusch do you plan to continue working on this PR?

@lionelkusch

Copy link
Copy Markdown
Contributor Author

Yes, I can take care of that if you agree on what you want displayed.

@Vincent-Maladiere

Copy link
Copy Markdown
Member

I think you can go ahead with my comment above

@lionelkusch

Copy link
Copy Markdown
Contributor Author

There are 2 displays one for the percentage and one with the full value.
The "abs value (percentile)" is for which display?

Additionally, I don't fully understand what you mean by "abs value (percentile)".
Can you give an example or the mathematical formula?

@Vincent-Maladiere

Vincent-Maladiere commented Jul 15, 2025

Copy link
Copy Markdown
Member

You can combine both. The height of each column corresponds to the total number of elements in the bin, and the text is the number of elements in the bin, with the percentile representation between parentheses. This applies to both numerical and categorical columns.

@lionelkusch

Copy link
Copy Markdown
Contributor Author

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.
This is linked to my warning that for the moment, the percentile is fixed in the number of characters, which simplifies the position of the text. Adding the number of elements creates some variability and creates a difficult problem for the position of the text relative to the bar.

In my opinion, it's better to stay with 2 representations: one with percentile and one with absolute numbers and switch between them.

@lionelkusch

Copy link
Copy Markdown
Contributor Author

This is an optimised version of the generation of the figures without generating twice the figures.

I released that it is not possible to duplicate or copy a figure of matplolib.
I decided to add the label at the end of the creation of the figure.
The addition of the label at the end allows me to add the label, serialise the figure and remove them just after getting the figure without them and restart.

I changed a bit of the label using the _utils for the format of the number and adjusted the position.
Tell me if the result is good for you.

@lionelkusch lionelkusch marked this pull request as ready for review July 16, 2025 16:41
@Vincent-Maladiere

Copy link
Copy Markdown
Member

Thanks for continuing this work. Overall, I'm still not convinced by the double plot approach, because:

  • The % box is hard to notice, so most people won't use it. Maybe we could mitigate this by adding the button to each plot? Anyway, I'm not fond of adding complexity to the UI.
  • This complexifies the logic significantly.
  • Performance-wise, I noted a 10% increase in compute time, which is not very significant. However, the disk footprint increases by 50%, which I still think is a lot for this feature, although the disk space itself is not big in absolute terms.

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?

@lionelkusch

Copy link
Copy Markdown
Contributor Author

I agree with you that this increase the complexity for the manage of the text and that there is an increase in memory for the generation of the second figure.

@rcap107

rcap107 commented Jul 18, 2025

Copy link
Copy Markdown
Member

...
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.

@lionelkusch

Copy link
Copy Markdown
Contributor Author

By your remarks, I see that the switching views is not interesting.
I propose a static version where the percentage and the value are represented at the same time in PR #1496.
I let you look at it for continuing the conversation.
I am sorry not to continue into this PR but I prefer to keep this PR focused only on the dynamic approach and the PR #1496 for static approaches. I hope you can understand me.

@rcap107

rcap107 commented Aug 28, 2025

Copy link
Copy Markdown
Member

By your remarks, I see that the switching views is not interesting. I propose a static version where the percentage and the value are represented at the same time in PR #1496. I let you look at it for continuing the conversation. I am sorry not to continue into this PR but I prefer to keep this PR focused only on the dynamic approach and the PR #1496 for static approaches. I hope you can understand me.

It makes sense, and thanks for working on both versions. We might revisit this version in the future, but I think that for the time being the static version would already be quite useful.

@rcap107 rcap107 linked an issue Oct 10, 2025 that may be closed by this pull request
@rcap107 rcap107 added the stalled This PR hasn't seen activity in some time and may be closed or handled by maintainers label Feb 16, 2026
@jeromedockes

Copy link
Copy Markdown
Member

in the end the alternative PR was merged so we can clsoe this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stalled This PR hasn't seen activity in some time and may be closed or handled by maintainers

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Showing values for bar plots in TableReport

5 participants