Update NullTransformer to make it user friendly #384

pvk-developer · 2022-02-07T12:56:30Z

NullTransformer

With the current PR the NullTransformer contains the following changes:

Always works with a copy of the data (no changes occur to the original data).
Adapt unit tests and integration tests.
New user friendly names for the fill value / nan and null column:
- fill_value or nan becomes -> missing_value_replacement.
- null_column becomes -> model_missing_values.

`missing_value_replacement`

By default this value will be None which will not replace the values. From python perspective this will return np.nan which should be the representation of Null.

The second strategy is the user specifying a string mode or mean , which will use the mode of the data or the mean value (if possible) as a replacement of the null values that are in the data.

The third strategy is the user specifying a value for it which will be used to fill the null values with.

`model_missing_values`

By default this value will be False. If set to True it will generate a new column representing if there was a missing value or not. In the case that the data had no missing values this column will not be generated.

Resolves #372 , #362 , #376

amontanez24

Looking good. I left a few comments

amontanez24 · 2022-02-07T19:12:04Z

rdt/transformers/null.py


-    def fit(self, data):
+    def fit(self, data, column):


If the only reason we are passing this is for the warning message then I'm not sure we need it. Couldn't we get the column name by doing data.name?

amontanez24 · 2022-02-07T19:13:20Z

rdt/transformers/null.py


-    def creates_null_column(self):
+    def creates_model_missing_values(self):


minor: I'd call this models_missing_values

tests/unit/transformers/test_base.py

amontanez24 · 2022-02-07T19:21:26Z

rdt/transformers/boolean.py

@@ -16,10 +16,10 @@ class BooleanTransformer(BaseTransformer):
    Null values are replaced using a ``NullTransformer``.

    Args:
-        nan (int or None):
+        missing_value_replacement (int or None):
            Replace null values with the given value. If ``None``, do not replace them.
            Defaults to ``-1``.


This isn't true anymore right?

rdt/performance/performance.py

amontanez24 · 2022-02-07T19:24:02Z

rdt/transformers/boolean.py

@@ -16,10 +16,10 @@ class BooleanTransformer(BaseTransformer):
    Null values are replaced using a ``NullTransformer``.

    Args:
-        nan (int or None):
+        missing_value_replacement (int or None):
            Replace null values with the given value. If ``None``, do not replace them.


I'd update this to match what's in the NullTransformer as well (ie. explain mode and so on)

codecov-commenter · 2022-02-08T12:44:27Z

Codecov Report

Merging #384 (ec32d26) into v1.0.0.dev (9507870) will not change coverage.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##           v1.0.0.dev      #384   +/-   ##
============================================
  Coverage      100.00%   100.00%           
============================================
  Files              12        12           
  Lines             942       935    -7     
============================================
- Hits              942       935    -7

Impacted Files	Coverage Δ
rdt/transformers/base.py	`100.00% <100.00%> (ø)`
rdt/transformers/boolean.py	`100.00% <100.00%> (ø)`
rdt/transformers/datetime.py	`100.00% <100.00%> (ø)`
rdt/transformers/null.py	`100.00% <100.00%> (ø)`
rdt/transformers/numerical.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9507870...ec32d26. Read the comment docs.

…to models missing values.

fealho

PR itself looks good, but I think you need to do another pass through the docstrings. I commented on the errors I saw, but you should probably double check the rest of the docstrings as well.

rdt/transformers/boolean.py

rdt/transformers/datetime.py

fealho · 2022-02-09T13:31:16Z

rdt/transformers/datetime.py

@@ -180,18 +182,19 @@ class DatetimeRoundedTransformer(DatetimeTransformer):
    This class behaves exactly as the ``DatetimeTransformer`` with ``strip_constant=True``.

    Args:
-        nan (int, str or None):
+        missing_value_replacement (int, str or None):


We should put object everywhere.

fealho · 2022-02-09T13:31:51Z

rdt/transformers/datetime.py

            Indicate what to do with the null values. If an integer is given, replace them
            with the given value. If the strings ``'mean'`` or ``'mode'`` are given, replace
            them with the corresponding aggregation. If ``None`` is given, do not replace them.
            Defaults to ``'mean'``.
-        null_column (bool):
+        model_missing_values (bool):


Delete the None docstring.

fealho · 2022-02-09T13:40:00Z

rdt/transformers/null.py

-        self.null_column = null_column
-        self.copy = copy
+    def __init__(self, missing_value_replacement=None, model_missing_values=False):
+        self.missing_value_replacement = missing_value_replacement


This should have an underscore in front(since you use self._missing_value_replacement around the code).

fealho · 2022-02-09T14:19:57Z

tests/unit/transformers/test_numerical.py

-        - First column of the input array rounded, replacing the indicated value with a Nan,
-          and kept as float values.
+        - First column of the input array rounded, replacing the indicated value with a
+          missing_value_replacement, and kept as float values.


dont change this.

fealho · 2022-02-09T14:20:44Z

tests/unit/transformers/test_numerical.py

        """Test the ``_transform`` method.

-        Validate that ``_transform`` produces the correct values when ``null_column`` is None.
+        Validate that ``_transform`` produces the correct values when ``model_missing_values``
+        is None.


not None, False.

fealho · 2022-02-09T14:21:01Z

tests/unit/transformers/test_numerical.py

@@ -1152,7 +1249,7 @@ def test__transform_null_column_none(self):
        ct = GaussianCopulaTransformer()
        ct._univariate = Mock()
        ct._univariate.cdf.return_value = np.array([0.25, 0.5, 0.75, 0.5])
-        ct.null_transformer = NullTransformer(None, null_column=None)
+        ct.null_transformer = NullTransformer('mean', model_missing_values=None)


fealho · 2022-02-09T14:21:17Z

tests/unit/transformers/test_numerical.py

@@ -1228,7 +1328,7 @@ def test__reverse_transform_null_column_none(self):
        ct = GaussianCopulaTransformer()
        ct._univariate = Mock()
        ct._univariate.ppf.return_value = np.array([0.0, 1.0, 2.0, 1.0])
-        ct.null_transformer = NullTransformer(None, null_column=None)
+        ct.null_transformer = NullTransformer(None, model_missing_values=None)


fealho · 2022-02-09T14:28:45Z

rdt/transformers/null.py

-    _null_column = None
-    _fill_value = None
+    _model_missing_values = None
+    _missing_value_replacement = False


I thought all variables should be set to None here? I'm not sure though.

amontanez24

One minor comment but once you finish addressing that and the other comments it should be good to go!

amontanez24 · 2022-02-09T18:19:12Z

rdt/transformers/boolean.py

-            If ``False``, do not create the new column.
+        missing_value_replacement (object or None):
+            Indicate what to do with the null values. If an object is given, replace them
+            with the given value. If the strings ``'mean'`` or ``'mode'`` are given, replace


I guess we should remove mean since that doesn't really make sense for booleans

fealho · 2022-02-09T19:17:20Z

tests/unit/transformers/test_numerical.py

@@ -129,7 +133,8 @@ def test__learn_rounding_digits_less_than_15_decimals(self):
        should be returned.

        Input:
-        - An array that contains floats with a maximum of 3 decimals and a NaN.
+        - An array that contains floats with a maximum of 3 decimals and a
+          missing_value_replacement.


After you put the NaN's back I think it's good to go.

fealho

A few more NaN's to change back.

fealho · 2022-02-09T19:28:28Z

tests/unit/transformers/test_numerical.py

@@ -594,8 +648,8 @@ def test__reverse_transform_rounding_none_with_nulls_dtype_int(self):
        Input:
        - 2d Array of multiple float values with decimals and a column setting at least 1 null.
        Output:
-        - First column of the input array rounded, replacing the indicated value with a Nan,


fealho · 2022-02-09T19:28:54Z

tests/unit/transformers/test_numerical.py

@@ -177,13 +182,13 @@ def test__learn_rounding_digits_negative_decimals_integer(self):

        assert output == -1

-    def test__learn_rounding_digits_all_nans(self):
-        """Test the _learn_rounding_digits method with data that is all NaNs.


fealho · 2022-02-09T19:28:59Z

tests/unit/transformers/test_numerical.py


-        If the data is all NaNs, expect that the output is None.


fealho · 2022-02-09T19:29:09Z

tests/unit/transformers/test_numerical.py


        Input:
-        - An array of NaNs.


pvk-developer added 9 commits February 4, 2022 12:26

Rename and remove copy

317a259

Add ability to not replace values

d23bd14

Set defaults not to replace missing values and not model missing values

aa81c81

Fix integration test

3ee4a62

Fix datetime tests

d4e31d6

Fix boolean tests

76bd9d1

Fix test numerical transformers

11111fb

Fix unit tests

42a0093

Fix test numerical

8b3318c

pvk-developer requested a review from a team as a code owner February 7, 2022 12:56

pvk-developer requested review from fealho and amontanez24 and removed request for a team February 7, 2022 12:56

pvk-developer added 3 commits February 7, 2022 16:09

Update quality and performance tests

3b951ab

Fix performance tests

f3e8ccc

Fix 100% coverage

2773277

amontanez24 requested changes Feb 7, 2022

View reviewed changes

pvk-developer force-pushed the issue_372_update_NullTransformer_to_new_api branch from 5901e9d to 1fd685b Compare February 8, 2022 20:03

pvk-developer added 4 commits February 8, 2022 21:03

Cap Scipy due to performance issues

1fd685b

Remove column from fit method of null transformer and rename creates …

662adc6

…to models missing values.

Fix nulltransformer docstring

4255236

Update docstrings

12168f9

fealho requested changes Feb 9, 2022

View reviewed changes

pvk-developer added 2 commits February 9, 2022 18:06

Address comments

15030aa

Fix lint

e97d286

amontanez24 approved these changes Feb 9, 2022

View reviewed changes

Fix docstrings

b7b36fb

fealho reviewed Feb 9, 2022

View reviewed changes

Update test docstrings

2f28f38

fealho approved these changes Feb 9, 2022

View reviewed changes

Replace missing_value_replacement with NaN

ec32d26

pvk-developer merged commit af4fc57 into v1.0.0.dev Feb 10, 2022

pvk-developer deleted the issue_372_update_NullTransformer_to_new_api branch February 10, 2022 01:25

This was referenced Feb 10, 2022

Update NullTransformer to make it user friendly #372

Closed

Update names & functionality for handling missing values #362

Closed

Rename columns parameter in fit for each individual transformer #376

Closed

pvk-developer added a commit that referenced this pull request Feb 28, 2022

Update NullTransformer to make it user friendly (#384)

b63f23d

amontanez24 pushed a commit that referenced this pull request Mar 7, 2022

Update NullTransformer to make it user friendly (#384)

c1a3b3a

amontanez24 pushed a commit that referenced this pull request Apr 21, 2022

Update NullTransformer to make it user friendly (#384)

90a1b19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update NullTransformer to make it user friendly #384

Update NullTransformer to make it user friendly #384

pvk-developer commented Feb 7, 2022

amontanez24 left a comment

amontanez24 Feb 7, 2022

amontanez24 Feb 7, 2022

amontanez24 Feb 7, 2022

amontanez24 Feb 7, 2022

codecov-commenter commented Feb 8, 2022 •

edited

Loading

fealho left a comment

fealho Feb 9, 2022

fealho Feb 9, 2022

fealho Feb 9, 2022

fealho Feb 9, 2022

fealho Feb 9, 2022

fealho Feb 9, 2022

fealho Feb 9, 2022

fealho Feb 9, 2022

amontanez24 left a comment

amontanez24 Feb 9, 2022

fealho Feb 9, 2022

fealho left a comment

fealho Feb 9, 2022

fealho Feb 9, 2022

fealho Feb 9, 2022

fealho Feb 9, 2022


		def creates_null_column(self):
		def creates_model_missing_values(self):

Update NullTransformer to make it user friendly #384

Update NullTransformer to make it user friendly #384

Conversation

pvk-developer commented Feb 7, 2022

NullTransformer

missing_value_replacement

model_missing_values

amontanez24 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Feb 8, 2022 • edited Loading

Codecov Report

fealho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amontanez24 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fealho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

`missing_value_replacement`

`model_missing_values`

codecov-commenter commented Feb 8, 2022 •

edited

Loading