Adding a threshold to DropColumnIfNull by rcap107 · Pull Request #1149 · skrub-data/skrub

rcap107 · 2024-11-21T10:27:31Z

Quick PR to address #1147

Notes:

I am using None to specify "Do not drop anything", rather than 0.0.
Do we want to change the name of the object? I don't think it's necessary.

(I also added fixed my account in the changelog)

Co-authored-by: Théo Jolivet <57430673+TheooJ@users.noreply.github.com>

…into drop_null_columns

…efault" This reverts commit 98b6c10.

jeromedockes

thanks for starting this @rcap107 !

skrub/_drop_column_if_null.py

jeromedockes · 2024-11-21T10:43:07Z

skrub/_drop_column_if_null.py

+
+    def __init__(self, threshold=1.0):
+        if threshold is not None:
+            assert 0.0 <= threshold <= 1.0, "Invalid value for the threshold."


can we show the value of the threshold in the error message? also use a valueerror rather than an assertionerror. assertions would be used to stop execution if a bug in skrub code has led to a situation that shouldn't be possible. valueerror is how we communicate to the user that the value they provided is inappropriate

in other words assertions are more of a debugging tool, not a way to validate user input. for example if the script is run with python -O or PYTHONOPTIMIZE=1 they are not executed

Noted, thanks for pointing that out. I changed it to a ValueError.

skrub/_drop_column_if_null.py

jeromedockes

thanks @rcap107 !! looks like we're close :)

jeromedockes · 2024-11-22T10:21:12Z

CHANGES.rst


-* Added a `DropColumnIfNull` transformer that drops columns that contain only null
-  values. :pr:`1115` by :user: `Riccardo Cappuzzo <riccardocappuzzo>`
+* Added a `DropColumnIfNull` transformer that drops columns based on how many


actually DropColumnIfNull has not been added to skrub's public API (and I'm not sure if it should before we decide what to do with selectors) so here we should document the added parameter to TableVectorizer rather than DroopColumnIfNull

jeromedockes · 2024-11-22T10:22:19Z

skrub/_drop_column_if_null.py

@@ -8,8 +8,22 @@


 class DropColumnIfNull(SingleColumnTransformer):


should we change the name to something like NullProportionThreshold ? 🤔

Maybe DropColumnWithNullThreshold? DropIfNullAboveThreshold?

maybe!
DropIfTooManyNulls?

+1 for DropIfTooManyNulls

skrub/_drop_column_if_null.py

jeromedockes · 2024-11-22T10:27:37Z

skrub/_drop_column_if_null.py

+        if self.threshold is not None:
+            if (
+                not (
+                    isinstance(self.threshold, float) or isinstance(self.threshold, int)


you can use

import numbers isinstance(self.threshold, numbers.Number)

instead.

it will work with numpy numeric types whereas

>>> isinstance(np.float32(0.5), float) False

Thanks, I knew that check didn't look right, but I wasn't sure how to write it properly

you're welcome! also next time you want a disjunction of isinstance checks you can put the types in a tuple

>>> isinstance(2, (str, int)) True >>> isinstance(2.5, (str, int)) False

jeromedockes

Thanks again @rcap107 !!
apart from maybe finalizing the choice of the name (of the transformer class and more importantly of the TableVectorizer parameter), this LGTM!

skrub/_table_vectorizer.py

jeromedockes · 2024-11-22T13:14:17Z

skrub/_table_vectorizer.py


-    drop_null_columns : bool, default=True
-        If set to `True`, columns that contain only null values are dropped.
+    null_threshold : float or None, default=1.0


@Vincent-Maladiere @GaelVaroquaux any opinions on this parameter's name?

drop_null_threshold maybe?

jeromedockes · 2024-11-22T13:17:41Z

glad that we will be able to include it in the next release. @rcap107 could you either mark it as "ready for review" or mention what you still plan to do before removing the "draft" status? thanks

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

rcap107 · 2024-11-22T13:19:32Z

Looks good to me! I have nothing else to add now.

Vincent-Maladiere

LGTM

Vincent-Maladiere · 2024-11-24T11:00:18Z

skrub/_drop_column_if_null.py

@@ -8,8 +8,22 @@


 class DropColumnIfNull(SingleColumnTransformer):


+1 for DropIfTooManyNulls

Vincent-Maladiere · 2024-11-24T11:05:34Z

skrub/_table_vectorizer.py


-    drop_null_columns : bool, default=True
-        If set to `True`, columns that contain only null values are dropped.
+    null_threshold : float or None, default=1.0


drop_null_threshold maybe?

Vincent-Maladiere · 2024-11-25T11:23:11Z

That is extremely cool :) let's merge this

rcap107 and others added 30 commits October 17, 2024 11:39

Adding code for DropNull

bee630f

Fixed line

ccc9a02

renamed script

d3f9c90

Added new common functions for drop and is_all_null

9d42b95

Fixed code

f249982

Added test for dropcol

a1caf39

Removing dev script

b0e3235

Update skrub/tests/test_dropnulls.py

90be825

Co-authored-by: Théo Jolivet <57430673+TheooJ@users.noreply.github.com>

Renamed file

55764a8

Renamed file

c8fdaaa

Formatting

0cdc0bd

Merge branch 'drop_null_columns' of https://github.com/rcap107/skrub …

34c0095

…into drop_null_columns

Rename file

430c8e3

Added docstrings

80bd408

Fixing imports and refactoring names

e2ca33f

Formatting

4dbba09

Updated changelog.

7d6f8ce

Formatting

4771d18

Removing function because it was not needed

f0b521a

Updated test

ea9893b

Merge branch 'main' into drop_null_columns

c73db7e

Improving tests

09cf9c7

Merge branch 'drop_null_columns' of https://github.com/rcap107/skrub …

4e4f255

…into drop_null_columns

Updated test

754e2ef

Merge remote-tracking branch 'main_repo/main' into drop_null_columns

acafac6

Fixed is_all_null based on comments

4b0aa1c

Renaming files for consistency

35f8909

Removing init

b4e419f

Moving DropNullColumn after CleanNullStrings

75f1110

Moved check on drop from transform to fit_transform

e499dc1

rcap107 added 5 commits November 18, 2024 15:26

Revert "Altering the code to add different options and changing the d…

801d745

…efault" This reverts commit 98b6c10.

Fixing changelog with correct account

ec37e13

Merge branch 'main' into drop_null_columns

04f3082

Quick update to add a threshold to dropnull

052c014

Fixed docstring and added error check

4d2bf13

rcap107 mentioned this pull request Nov 21, 2024

Add a threshold to DropColumnIfNull #1147

Closed

rcap107 added 2 commits November 21, 2024 11:31

Fixing error checking, the irony

d6b9b6e

Removing type hint

de9d1fc

jeromedockes reviewed Nov 21, 2024

View reviewed changes

rcap107 added 5 commits November 21, 2024 14:30

Addressed review and added tests

a2ef721

Fixed docs, added a test

29ffd73

Improved docstring and error checking, refactoring

01446ba

Fixing bug with error checking

45f77bb

Updating changelog

c40135c

jeromedockes added this to the 0.3.2 milestone Nov 21, 2024

jeromedockes reviewed Nov 22, 2024

View reviewed changes

Updating TableVectorizer to use the new parameter

f67fc31

jeromedockes approved these changes Nov 22, 2024

View reviewed changes

Update skrub/_table_vectorizer.py

d58e7ff

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

rcap107 marked this pull request as ready for review November 22, 2024 13:19

Formatting

4b861d2

Vincent-Maladiere approved these changes Nov 24, 2024

View reviewed changes

rcap107 added 3 commits November 25, 2024 11:31

Renaming

1f78684

Renaming tests

2a4f804

Fixing changelog

72c1fe6

jeromedockes merged commit 1967d51 into skrub-data:main Nov 25, 2024

rcap107 deleted the drop_null_columns branch November 25, 2024 15:47

		@@ -8,8 +8,22 @@


		class DropColumnIfNull(SingleColumnTransformer):

Conversation

rcap107 commented Nov 21, 2024

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!

Uh oh!

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!

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!

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!

jeromedockes commented Nov 22, 2024

Uh oh!

rcap107 commented Nov 22, 2024

Uh oh!

Vincent-Maladiere 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!

Vincent-Maladiere commented Nov 25, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants