Skip Plotting in Table Report Based on Max Columns Given by priscilla-b · Pull Request #1255 · skrub-data/skrub

priscilla-b · 2025-03-18T13:31:03Z

…eneration behavior

jeromedockes

this is great, thank you very much @priscilla-b ! especially when we are inspecting intermediate steps in a pipeline, we often end up with dataframes that have a lot of columns and this will allow checking them without waiting a long time to generate lots of plots we won't look at 🚀

In fact it is so useful that I think the default should not be None but maybe 30 or something like that. but that would require adding clear messages in the report itself telling users what to do if they really want the plots, so it could be left for another pull request.

jeromedockes · 2025-03-19T08:35:43Z

-        if "_summary_with_plots" in self.__dict__:
-            return self._summary_with_plots
-        return self._summary_without_plots
+        if self.max_plot_columns is None:


this is a great way of handling it! I think we should rename _any_summary and make it a regular method rather than a property. maybe self._any_summary -> self._get_summary()?

I thought of this but wasn't sure the benefits of having it as a method than a property, so decided to maintain the existing implementation

sorry for the nitpick: could we rename get_summary to _get_summary? this is not something for users and we want the freedom to change the contents of the summary whenever we want so the leading underscore indicates it's not part of the public API

GaelVaroquaux · 2025-03-19T09:10:14Z

In fact it is so useful that I think the default should not be None but maybe 30 or something like that.

+1

but that would require adding clear messages in the report itself telling users what to do if they really want the plots, so it could be left for another pull request.

Indeed a message visible somewhere (like the tab being greyed out and when the mouse goes over it a box appears giving the message). But probably for another PR because it requires markedly more work.

Co-authored-by: Jérôme Dockès <jerome@dockes.org>

priscilla-b · 2025-03-19T23:00:07Z

Will look into the issue of changing the default param to some arbitrary number

… skip-plot

jeromedockes · 2025-03-28T11:08:57Z

very nice @priscilla-b ! thanks for adding the message and doing the extra work to make it dismissable!

jeromedockes

thanks! I know it's still in draft mode but took a quick look anyway :)

this really speeds up the report, now when there are many columns the bottleneck is definitely the computation of column associations

jeromedockes · 2025-03-28T11:11:37Z

+            this.elem.querySelector("[data-role='dismiss-button']")
+                .addEventListener("click", (e) => this.hide(e));
+        }
+
+        hide(event) {
+            this.elem.style.display = "none";
+        }


Suggested change

this.elem.querySelector("[data-role='dismiss-button']")

.addEventListener("click", (e) => this.hide(e));

}

hide(event) {

this.elem.style.display = "none";

}

this.elem.querySelector("[data-role='dismiss-button']")

.addEventListener("click", (e) => this.hide());

}

I think here we can rely on the parent classe's hide() to hide the whole element and it simplifies things slightly?

jeromedockes · 2025-03-28T11:14:41Z



-def patch_display(pandas=True, polars=True, verbose=1):
+def patch_display(pandas=True, polars=True, verbose=1, max_plot_columns=None):


here also we could put 30 instead of None as the default

jeromedockes · 2025-03-28T11:17:00Z

    summary["n_constant_columns"] = sum(
        c["value_is_constant"] for c in summary["columns"]
    )
+    summary["plots_skipped"] = all(not c["plot_names"] for c in summary["columns"])


I think we can define this based on the argument rather than inspecting the results, something like summary["plots_skipped"] = not with_plots. I think it is a bit simpler and slightly more robust (for example in the weird edge case where all columns are constant there will be no plots but not because they are skipped just because there is nothing to plot). WDYT?

Yes I think I did this at first, but I thought we were just reverting the argument and not really checking the presence of the actual plots. But yeah, what I did makes it more complicated, and should just keep it simple.

jeromedockes · 2025-03-28T11:18:31Z

-        if "_summary_with_plots" in self.__dict__:
-            return self._summary_with_plots
-        return self._summary_without_plots
+        if self.max_plot_columns is None:


sorry for the nitpick: could we rename get_summary to _get_summary? this is not something for users and we want the freedom to change the contents of the summary whenever we want so the leading underscore indicates it's not part of the public API

jeromedockes · 2025-03-28T11:24:45Z

+        # there are too many columns
+        # Will be accessed in the HTML template to display a message to the user
+        summary["plots_auto_skipped"] = (
+            summary["plots_skipped"] and self.max_plot_columns == 30


I'm not sure why we compare max_plot_columns to 30 here? for example if the user sets it to 20 it would still be skipped?

maybe the template can look directly at plots_skipped which you add in summarize, or we could create a flag in the conditional blocks above that we then use eg

... summary = self._summary_with_plots plots_skipped = False else: summary = self._summary_without_plots plots_skipped = True summary['plots_auto_skipped'] = plots_skipped

I was thinking that if the user set the param to skip the plots directly, they wouldn't need to be shown a message explaining why the plots were skipped.
I wanted to show the message only when the plots were automatically skipped, but couldn't really figure out a simple implementation for checking if the default value is being used or not

aah ok I understand now, thanks. I didn't realize the goal was to avoid showing the message when the user intentionally skipped plotting by setting max_plot_columns.

I think it might be good to show the message whenever plotting was skipped, even if it is intentional, especially since you made it easy to dismiss the message but both options make sense

Vincent-Maladiere · 2025-03-31T15:25:38Z

Hey @priscilla-b, thanks for working on this! It seems like the tiles without distributions are somewhat redundant with the 'stats' panel (the only difference being the IQR). Would it make sense to remove them entirely and display only your message in the distribution panel?

jeromedockes · 2025-03-31T15:41:44Z

It seems like the tiles without distributions are somewhat redundant with the 'stats' panel

that's a good point. Still I would like to have this PR for the next release so maybe we should finish the few remaining details for the current version and open a separate issue for follow-ups?

priscilla-b · 2025-03-31T15:47:25Z

Hi Jerome, Yes I will complete this and reopen by close of day.

…

On Mon, 31 Mar 2025 at 3:42 PM, Jérôme Dockès ***@***.***> wrote: It seems like the tiles without distributions are somewhat redundant with the 'stats' panel that's a good point. Still I would like to have this PR for the next release so maybe we should finish the few remaining details for the current version and open a separate issue for follow-ups? — Reply to this email directly, view it on GitHub <#1255 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/APZQ2LXYUTN7UYCZTTGA7U32XFO47AVCNFSM6AAAAABZIFLBSWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDONRWGYZTGNBTGY> . You are receiving this because you were mentioned.Message ID: ***@***.***> [image: jeromedockes]*jeromedockes* left a comment (skrub-data/skrub#1255) <#1255 (comment)> It seems like the tiles without distributions are somewhat redundant with the 'stats' panel that's a good point. Still I would like to have this PR for the next release so maybe we should finish the few remaining details for the current version and open a separate issue for follow-ups? — Reply to this email directly, view it on GitHub <#1255 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/APZQ2LXYUTN7UYCZTTGA7U32XFO47AVCNFSM6AAAAABZIFLBSWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDONRWGYZTGNBTGY> . You are receiving this because you were mentioned.Message ID: ***@***.***>

Vincent-Maladiere · 2025-03-31T15:47:40Z

Right, let's do this in two steps then 👍

…lots

Vincent-Maladiere

Looks good on VSCode!

jeromedockes

LGTM! Thanks again @priscilla-b 🎉

…1255) Co-authored-by: Jérôme Dockès <jerome@dockes.org>

Co-authored-by: Jérôme Dockès <jerome@dockes.org>

priscilla-b added 6 commits March 17, 2025 08:28

apply pre-commit formats

b6a0dde

correct logic in max_plot_columns condition for summary selection

004b975

add max_plot_columns parameter to patch_display and _patch functions

e783753

update documentation for max_plot_columns parameter to clarify plot g…

1d86cb6

…eneration behavior

add unit tests for max_plot_columns parameter in TableReport

8b1e59c

add contribution text

1aff270

jeromedockes reviewed Mar 19, 2025

View reviewed changes

jeromedockes mentioned this pull request Mar 19, 2025

Some issues with TableReport I noticed while working with many columns #1257

Closed

priscilla-b and others added 2 commits March 19, 2025 22:51

use skrub utility method to compute number of columns

f334413

Co-authored-by: Jérôme Dockès <jerome@dockes.org>

add sbd import

77a8d0c

Merge branch 'main' into skip-plot

b948680

jeromedockes added the TableReport anything related to the TableReport label Mar 20, 2025

priscilla-b added 5 commits March 23, 2025 21:12

rename _any_summary property to get_summary method

ce683f4

replace class summary with method

1c8f9ea

set default plot cols to 30

51208f8

add check for presence of plots in summary

5d41ad1

Merge branch 'skip-plot' of https://github.com/priscilla-b/skrub into…

daee0e2

… skip-plot

priscilla-b marked this pull request as draft March 25, 2025 07:40

priscilla-b added 3 commits March 27, 2025 16:29

show alert message when plots are automatically skipped

6497a20

add get_summary doc string

5dcc67d

update doc string

b83139e

jeromedockes reviewed Mar 28, 2025

View reviewed changes

Vincent-Maladiere mentioned this pull request Mar 31, 2025

Remove distribution tiles when distributions aren't computed #1267

Closed

jeromedockes added this to the 0.5.2 milestone Apr 1, 2025

priscilla-b and others added 5 commits April 2, 2025 12:49

enhance plot column checks

4f1c2b2

update default value for max_plot_column param

93149f4

refactor: simplify hide method and update summary logic for skipped p…

aabd3c2

…lots

Merge branch 'main' into skip-plot

6647b5c

format file

37e4a1a

priscilla-b marked this pull request as ready for review April 2, 2025 13:17

add cases for max_plot_columns parameter with None value

2756b34

Vincent-Maladiere approved these changes Apr 3, 2025

View reviewed changes

jeromedockes approved these changes Apr 3, 2025

View reviewed changes

jeromedockes merged commit 1373ba3 into skrub-data:main Apr 3, 2025

rcap107 pushed a commit to rcap107/skrub that referenced this pull request Apr 3, 2025

Skip Plotting in Table Report Based on Max Columns Given (skrub-data#…

0d65020

…1255) Co-authored-by: Jérôme Dockès <jerome@dockes.org>

rcap107 pushed a commit to rcap107/skrub that referenced this pull request Apr 3, 2025

Skip Plotting in Table Report Based on Max Columns Given (skrub-data#…

8783ce2

…1255) Co-authored-by: Jérôme Dockès <jerome@dockes.org>

rcap107 pushed a commit that referenced this pull request Apr 3, 2025

Skip Plotting in Table Report Based on Max Columns Given (#1255)

db562e1

Co-authored-by: Jérôme Dockès <jerome@dockes.org>



		def patch_display(pandas=True, polars=True, verbose=1):
		def patch_display(pandas=True, polars=True, verbose=1, max_plot_columns=None):

Uh oh!

Conversation

priscilla-b commented Mar 18, 2025

Uh oh!

jeromedockes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GaelVaroquaux commented Mar 19, 2025 via email

Uh oh!

priscilla-b commented Mar 19, 2025

Uh oh!

jeromedockes commented Mar 28, 2025

Uh oh!

jeromedockes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Vincent-Maladiere commented Mar 31, 2025

Uh oh!

jeromedockes commented Mar 31, 2025

Uh oh!

priscilla-b commented Mar 31, 2025 via email

Uh oh!

Vincent-Maladiere commented Mar 31, 2025

Uh oh!

Vincent-Maladiere left a comment

Choose a reason for hiding this comment

Uh oh!

jeromedockes left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants