Support multi-column transformers #692

R-Palazzo · 2023-08-22T12:54:30Z

Resolve #683

amontanez24 · 2023-08-23T16:26:24Z

rdt/transformers/base.py

+            data (pandas.DataFrame):
+                The entire table.
+        """
+        column_names = columns


what's the point of this assignment? Why not just change the name of the variable

Yes first I was not sure if we really wanted to modify the column parameter, but I removed my change, it was not necessary

amontanez24 · 2023-08-23T16:27:17Z

rdt/hyper_transformer.py

-        for field in self._input_columns:
-            data = self._fit_field_transformer(data, field, self.field_transformers[field])
+        for field_column, field_transformer in self.field_transformers.items():
+            data = self._fit_field_transformer(data, field_column, field_transformer)


I don't think this is going to work alone. The _fit_field_transformer method doesn't know to pass the columns as a dict in the multi column case

In the multi column case we pass the columns as a tuple or as a dict? I supposed it was a tuple and so it was good for me based on the docstring of _fit_field_transformer(self, data, field, transformer):

field (str or tuple): Name of column or tuple of columns in data that will be transformed

In the base class you made fit takes in data and a dictionary. It needs to be a dict otherwise the values will always have to be passed in a specific order

@R-Palazzo I think this is still the case. The issue states that the HyperTransformer should run with a multi-column transformer too.

codecov-commenter · 2023-09-04T15:19:46Z

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (5d7f8b7) 100.00% compared to head (c0d6294) 100.00%.
Report is 3 commits behind head on multi-column-transformer.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@                    Coverage Diff                     @@
##           multi-column-transformer      #692   +/-   ##
==========================================================
  Coverage                    100.00%   100.00%           
==========================================================
  Files                            17        17           
  Lines                          1799      1862   +63     
==========================================================
+ Hits                           1799      1862   +63

Files Changed	Coverage Δ
rdt/hyper_transformer.py	`100.00% <100.00%> (ø)`
rdt/transformers/__init__.py	`100.00% <100.00%> (ø)`
rdt/transformers/base.py	`100.00% <100.00%> (ø)`
rdt/transformers/utils.py	`100.00% <100.00%> (ø)`

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

amontanez24 · 2023-09-08T18:29:35Z

rdt/hyper_transformer.py

-        for field in self._input_columns:
-            data = self._fit_field_transformer(data, field, self.field_transformers[field])
+        for field_column, field_transformer in self.field_transformers.items():
+            data = self._fit_field_transformer(data, field_column, field_transformer)


@R-Palazzo I think this is still the case. The issue states that the HyperTransformer should run with a multi-column transformer too.

amontanez24 · 2023-09-08T18:30:06Z

tests/integration/transformers/test_base.py

+def test_multi_column_transformer_same_number_of_columns_input_output():
+    """Test a multi-column transformer when the same of input and output columns."""
+    # Setup
+    class AdditionTransformer(BaseMultiColumnTransformer):


I think we should actually test this example from the HyperTransformer and make sure it really works end to end

amontanez24 · 2023-09-08T18:31:48Z

rdt/transformers/base.py

+            if is_prefix_dict:
+                prefix = self.prefixes[output_column]
+            else:
+                prefix = self.prefixes


Why is this allowing multiple types for the prefixes? I think it could be complicated if developers have to set custom prefixes and output names in 2 places. Maybe it would be simpler to say that if you want to have custom prefixes for each output name, then just use custom output names and set the prefix to None.

In other words, _get_prefix always returns None or a string. And then you can just do the lines below

amontanez24 · 2023-09-08T18:39:24Z

rdt/transformers/base.py

+        missing = set(columns_to_sdtypes.keys()) - set(data.columns)
+        if missing:
+            missing_to_print = ', '.join(missing)
+            raise KeyError(f'Columns ({missing_to_print}) are not present in the data.')


I don't think this should be a KeyError. I think that's really only for if the accessed object doesn't have the key. I think maybe a ValueError makes sense since columns_to_sdtypes has the wrong values

amontanez24 · 2023-09-08T18:46:23Z

tests/unit/transformers/test_base.py

+        transformer._get_prefix.assert_called_once_with()
+
+    def test__validate_columns_to_sdtypes(self):
+        """Test the ``_validate_columns_to_sdtypes`` method."""


nitpick: for these descriptions I think it's more useful to say what's being tested. eg

Test that an error with the missing column names is raised.

pvk-developer

LGTM!

pvk-developer · 2023-09-11T15:06:23Z

rdt/hyper_transformer.py

+        columns_to_sdtypes = {}
+        for column in field:
+            columns_to_sdtypes[column] = self.field_sdtypes[column]
+        return columns_to_sdtypes


Break line above would be appreciated.

amontanez24

LGTM! Thanks for addressing

* def + store_columns * hypertransformer modif 1 * unit test * docstring * drop _store_columns change * add integration_test + _get_output_to_property * ordered_columns + prefix * 'int' in integration test * change prefix + columns_to_sdtype * docstring * columns_to_sdtypes * validate column_to_sdtype ValueError + docstring * fix _get_prefix * move PR, hypertransformer with multi column * break line

R-Palazzo requested review from amontanez24 and frances-h August 22, 2023 12:54

R-Palazzo requested a review from a team as a code owner August 22, 2023 12:54

R-Palazzo removed the request for review from a team August 22, 2023 12:54

amontanez24 reviewed Aug 23, 2023

View reviewed changes

R-Palazzo force-pushed the issue-683-support-multi-column-transformers branch from 1afbe42 to 5a1feb8 Compare August 24, 2023 13:32

R-Palazzo added 7 commits September 7, 2023 09:41

def + store_columns

febb95b

hypertransformer modif 1

b0ad789

unit test

44f5cc1

docstring

18d48ad

drop _store_columns change

43b2fa0

add integration_test + _get_output_to_property

be30d90

ordered_columns + prefix

054d9eb

R-Palazzo force-pushed the issue-683-support-multi-column-transformers branch from ce0e2a2 to 054d9eb Compare September 7, 2023 07:42

R-Palazzo added 3 commits September 7, 2023 10:30

'int' in integration test

afbf35d

change prefix + columns_to_sdtype

acfd2c8

docstring

9153334

R-Palazzo requested review from pvk-developer and removed request for frances-h September 8, 2023 08:12

columns_to_sdtypes

ba302ba

amontanez24 requested changes Sep 8, 2023

View reviewed changes

R-Palazzo added 3 commits September 11, 2023 08:15

validate column_to_sdtype ValueError + docstring

83af68f

fix _get_prefix

e8e2d9d

move PR, hypertransformer with multi column

8f88b1e

R-Palazzo changed the base branch from master to multi-column-transformer September 11, 2023 14:54

pvk-developer approved these changes Sep 11, 2023

View reviewed changes

amontanez24 approved these changes Sep 11, 2023

View reviewed changes

break line

c0d6294

R-Palazzo merged commit 89d871d into multi-column-transformer Sep 12, 2023
46 checks passed

R-Palazzo deleted the issue-683-support-multi-column-transformers branch September 12, 2023 15:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support multi-column transformers #692

Support multi-column transformers #692

R-Palazzo commented Aug 22, 2023

amontanez24 Aug 23, 2023

R-Palazzo Aug 24, 2023

amontanez24 Aug 23, 2023

R-Palazzo Aug 24, 2023

amontanez24 Aug 24, 2023

amontanez24 Sep 8, 2023

codecov-commenter commented Sep 4, 2023 •

edited

Loading

amontanez24 Sep 8, 2023

amontanez24 Sep 8, 2023

amontanez24 Sep 8, 2023

amontanez24 Sep 8, 2023

amontanez24 Sep 8, 2023

pvk-developer left a comment

pvk-developer Sep 11, 2023

amontanez24 left a comment

Support multi-column transformers #692

Support multi-column transformers #692

Conversation

R-Palazzo commented Aug 22, 2023

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

codecov-commenter commented Sep 4, 2023 • edited Loading

Codecov Report

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

pvk-developer left a 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

codecov-commenter commented Sep 4, 2023 •

edited

Loading