Skip to content

Conversation

@jeromedockes
Copy link
Member

moving skrubview into skrub

@jeromedockes jeromedockes marked this pull request as draft July 2, 2024 10:03
@GaelVaroquaux
Copy link
Member

Nitpick: I think that it should be called "TableReport" :)

@jeromedockes
Copy link
Member Author

jeromedockes commented Jul 2, 2024 via email

@jeromedockes
Copy link
Member Author

ok I'll have another look tomorrow but I think I have addressed most comments from @GaelVaroquaux

@GaelVaroquaux
Copy link
Member

Do you want to work on your on-line visualizer and add it in the docs, or should we rather merge before (probably a good idea)?

@jeromedockes
Copy link
Member Author

jeromedockes commented Jul 15, 2024 via email

@GaelVaroquaux
Copy link
Member

Failing test (a quick look suggests that it is related to the PR).

Playing with the report, I just had an idea of what I think is a useful functionality that's probably very easy to implement: if there are very similar columns (threshold to be defined, I'd say .9), add in the drop-down menu that selects columns to display an entry named "very similar columns" that would only select columns that have a similarity measure with another column above the threshold.

@jeromedockes
Copy link
Member Author

jeromedockes commented Jul 15, 2024 via email

@GaelVaroquaux
Copy link
Member

I like it! Indeed it's just a question of adding a list of those column names in a dictionary somewhere so I'll do it in this PR

Awesome. I definitely see myself using this functionality

@jeromedockes
Copy link
Member Author

jeromedockes commented Jul 15, 2024 via email

@GaelVaroquaux
Copy link
Member

GaelVaroquaux commented Jul 15, 2024 via email

@jeromedockes
Copy link
Member Author

you can see the new filter in action here

the matplotlib thingy is fixed too so the PR should be ok now

Copy link
Member

@GaelVaroquaux GaelVaroquaux left a comment

Choose a reason for hiding this comment

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

Two tiny comments and then merge

# anything with them; cast raises an exception.
# polars emits a performance warning when using map_elements
with warnings.catch_warnings():
warnings.simplefilter("ignore")
Copy link
Member

Choose a reason for hiding this comment

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

I think that it would be nice if we where a bit more specific in the warning that we catch

@jeromedockes jeromedockes merged commit d9fefbc into skrub-data:main Jul 16, 2024
@jeromedockes jeromedockes deleted the add-reporting branch July 16, 2024 10:21
@GaelVaroquaux
Copy link
Member

Hurray, merged!!!

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.

3 participants