Improve user warnings and logic for update_transformers and update_transformers_by_sdtype #695

R-Palazzo · 2023-08-24T14:50:14Z

Resolve #685
I’m not using the BaseMultiColumnTransformer because it’s crashing on some validation. It’s mainly because this transformer does not have a supported_sdtype. So to check what I’m doing, I’m using the option to assign to None the transformers. Let me know what we should do about this

Also, about the update_transformers_by_sdtype I made the change according to what is written in the issue; there is one case I’m not sure about. For me, when we update the sdtype we should not touch all the columns that have this sdtype but that are used in a Multi Column transformer. In this PR I do it, the columns are removed to the tuple and are being assigned to the new transformer.

tests/integration/test_hyper_transformer.py

rdt/hyper_transformer.py

amontanez24 · 2023-08-25T16:39:38Z

rdt/hyper_transformer.py

@@ -335,6 +375,8 @@ def update_transformers_by_sdtype(
        self._warn_update_transformers_by_sdtype(transformer, transformer_name)

        transformer_instance = transformer
+        self._multi_column_fields = self._create_multi_column_fields()


we should just remove this from the init and move it into the detect method. Then we don't need to keep calling it

codecov-commenter · 2023-09-04T17:03:41Z

Codecov Report

❗ No coverage uploaded for pull request base (multi-column-transformer@89d871d). Click here to learn what that means.
Patch has no changes to coverable lines.

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

Additional details and impacted files

@@                     Coverage Diff                      @@
##             multi-column-transformer      #695   +/-   ##
============================================================
  Coverage                            ?   100.00%           
============================================================
  Files                               ?        17           
  Lines                               ?      1884           
  Branches                            ?         0           
============================================================
  Hits                                ?      1884           
  Misses                              ?         0           
  Partials                            ?         0

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

fealho

Looks pretty good, just had a couple of comments

rdt/hyper_transformer.py

fealho · 2023-09-05T20:36:44Z

rdt/hyper_transformer.py

@@ -205,7 +211,19 @@ def _validate_config(config):

        sdtypes = config['sdtypes']
        transformers = config['transformers']
-        if set(sdtypes.keys()) != set(transformers.keys()):
+
+        sdtype_keys = sdtypes.keys()


You can't have a dict with repeated keys, so I don't think the is_sdtype_keys_unique check if necessary.

fealho · 2023-09-05T20:41:12Z

tests/integration/test_hyper_transformer.py

+
+    def test_update_transformers_single_to_multi_column(self):
+        """Test ``update_transformers`` to go from single to mutli column transformer."""
+


fealho · 2023-09-05T20:42:05Z

tests/unit/test_hyper_transformer.py

+            'B': None,
+            'C': FloatFormatter(),
+        }
+        ht.field_transformers == expected_field_transformers


tests/unit/test_hyper_transformer.py

fealho · 2023-09-05T20:42:19Z

tests/unit/test_hyper_transformer.py

+            ('A', 'B'): None,
+            'C': None,
+        }
+        ht.field_transformers == expected_field_transformers


amontanez24

This looks pretty good! I do think we should move the logic to make the HyperTransformer work with a multi-column transformer to the other PR though. It makes more sense with that issue

rdt/hyper_transformer.py

amontanez24 · 2023-09-08T18:58:25Z

rdt/hyper_transformer.py

+    def _remove_column_in_multi_column_fields(self, column):
+        """Remove a column that is part of a multi-column field.
+
+        Update the tuple to remove the column from it and modify the ``multi_column_fields``


minor: you can just say

Remove the column from the tuple and ...

amontanez24 · 2023-09-08T18:59:16Z

rdt/hyper_transformer.py

+        new_tuple = tuple(item for item in old_tuple if item != column)
+
+        if len(new_tuple) == 1:
+            new_tuple, = new_tuple


what is the point of this line?

It's to not have a single element tuple in the field_transformer dict. Without this line I have a test failing with the error:
AssertionError: assert {('column2',): 'transformer'} == {'column2': 'transformer'}
The right is the expected dict

amontanez24 · 2023-09-08T19:05:34Z

rdt/hyper_transformer.py

+            if isinstance(field, tuple):
+                columns_to_sdtypes = self._get_columns_to_sdtypes(field)
+                transformer.fit(data, columns_to_sdtypes)
+            else:
+                transformer.fit(data, field)


I think this should be moved to the base PR

amontanez24

LGTM!

…ansformers_by_sdtype (#695) * modify update_transformer * modify validate_config * modify config printing * add _generate_column_in_tuple + update transformer * add flatten_column_list * add _update_column_in_tuple method * update_transformers_by_sdtype * integration test * unit tests * rename * docstring * remove _create_multi_column_fields in the __init__ * fix rebasing * use a multi column transformer in integration test * assert + extra line * rebase + test end to end with hypertransformer * add _get_columns_to_sdtype * rebase * remove notebook * undo remove unit test * 'Remove the column from the tuple and' docstring

R-Palazzo requested review from amontanez24 and fealho August 24, 2023 14:50

R-Palazzo requested a review from a team as a code owner August 24, 2023 14:50

R-Palazzo removed the request for review from a team August 24, 2023 14:50

R-Palazzo force-pushed the issue-685-update-transformers branch from d9bdfbf to 2147a08 Compare August 24, 2023 14:52

amontanez24 reviewed Aug 25, 2023

View reviewed changes

R-Palazzo force-pushed the issue-685-update-transformers branch from 2147a08 to e8d956c Compare September 4, 2023 17:00

fealho reviewed Sep 5, 2023

View reviewed changes

fealho approved these changes Sep 6, 2023

View reviewed changes

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

R-Palazzo force-pushed the issue-685-update-transformers branch 3 times, most recently from d8ba6e1 to daf2cbe Compare September 8, 2023 08:00

amontanez24 reviewed Sep 8, 2023

View reviewed changes

R-Palazzo force-pushed the issue-685-update-transformers branch from daf2cbe to 4ba050d Compare September 11, 2023 08:37

Base automatically changed from issue-683-support-multi-column-transformers to multi-column-transformer September 12, 2023 15:55

R-Palazzo added 13 commits September 13, 2023 09:38

modify update_transformer

1f3f0d7

modify validate_config

6d655ae

modify config printing

df4fbe9

add _generate_column_in_tuple + update transformer

09352af

add flatten_column_list

598a18d

add _update_column_in_tuple method

1e023b1

update_transformers_by_sdtype

1b3966e

integration test

0870154

unit tests

f0f3248

rename

ef714af

docstring

1eb8784

remove _create_multi_column_fields in the __init__

dea4428

fix rebasing

74c91bc

R-Palazzo added 7 commits September 13, 2023 09:38

use a multi column transformer in integration test

0312b8c

assert + extra line

807bcaf

rebase + test end to end with hypertransformer

b44a667

add _get_columns_to_sdtype

3153140

rebase

76d8817

remove notebook

645a01a

undo remove unit test

9494190

R-Palazzo force-pushed the issue-685-update-transformers branch from 75e87c2 to 9494190 Compare September 13, 2023 07:39

'Remove the column from the tuple and' docstring

87231cc

amontanez24 approved these changes Sep 13, 2023

View reviewed changes

R-Palazzo merged commit 0cb92e8 into multi-column-transformer Sep 13, 2023
46 checks passed

R-Palazzo deleted the issue-685-update-transformers branch September 13, 2023 16:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve user warnings and logic for update_transformers and update_transformers_by_sdtype #695

Improve user warnings and logic for update_transformers and update_transformers_by_sdtype #695

R-Palazzo commented Aug 24, 2023

amontanez24 Aug 25, 2023

codecov-commenter commented Sep 4, 2023 •

edited

Loading

fealho left a comment

fealho Sep 5, 2023

fealho Sep 5, 2023

fealho Sep 5, 2023

fealho Sep 5, 2023

amontanez24 left a comment

amontanez24 Sep 8, 2023

amontanez24 Sep 8, 2023

R-Palazzo Sep 13, 2023

amontanez24 Sep 8, 2023

amontanez24 left a comment


		def test_update_transformers_single_to_multi_column(self):
		"""Test ``update_transformers`` to go from single to mutli column transformer."""

Improve user warnings and logic for update_transformers and update_transformers_by_sdtype #695

Improve user warnings and logic for update_transformers and update_transformers_by_sdtype #695

Conversation

R-Palazzo commented Aug 24, 2023

Choose a reason for hiding this comment

codecov-commenter commented Sep 4, 2023 • 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

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

amontanez24 left a comment

Choose a reason for hiding this comment

codecov-commenter commented Sep 4, 2023 •

edited

Loading