Add utility function to simplify multi-table schemas #1874

R-Palazzo · 2024-03-26T21:54:22Z

CU-86azkea74
Resolve #1832

Formula to determine how many columns to drop for a child

$n$: number of columns of the table
$k$: number of parameter columns per column. For beta's distribution $k=4$
$m = \frac{1000}{\text{total number of relationship}}$: maximum number of column per relationship
minimum number of column to drop = $n + k - \sqrt{k^2 + 1 + 2m}$

Some other info:

I updated some methods inside HMA to be class or static methods so I can use them outside HMA. I preferred keeping them inside HMA, though, because they are very specific to this model (not used for HSA, for instance.)
There is an integration test over our biggest demo datasets.
The method _simplify_data(data, metadata) makes your data match your metadata.
It can be useful when you're updating your metadata (removing columns and tables...) to make it work along with your data. If we want, it can be public.

sdv-team · 2024-03-26T21:54:24Z

Task linked: CU-86azkea74 SDV - Add utility function to simplify multi-table schemas #1832

amontanez24

This is looking pretty good! I think in a couple of places we can maybe reduce the number of functions and simplify the logic a few steps. It would be helpful to add a docstring for the _get_num_column_to_drop so we can discuss it more

Also in general can we fix some of the names:

childs -> children
grandchilds -> grandchildren
simplify -> simplified if it is being used as an adjective

sdv/multi_table/hma.py

sdv/multi_table/utils.py

amontanez24 · 2024-03-28T21:15:15Z

sdv/multi_table/utils.py

+    all_descendants_to_keep = all_descendants_per_root[root_with_max_descendants]
+
+    simplify_metadata, childs, grandchilds = _simplify_relationships(
+        simplify_metadata, root_with_max_descendants, all_descendants_to_keep
+    )
+
+    simplify_metadata = _simplify_non_descendants_tables(
+        simplify_metadata, root_with_max_descendants, all_descendants_to_keep
+    )


could this be simplified by:

Getting a list of all table names that are within 2 of the root (all of the root's children or grandchildren)

Looping through all tables and pruning the ones that aren't in that list

sdv/multi_table/utils.py

frances-h · 2024-03-28T20:25:09Z

tests/integration/utils/test_poc.py

+def test_simplify_schema_invalid_metadata():
+    """Test ``simplify_schema`` when the metadata is not invalid."""
+    # Setup
+    real_data, metadata = download_demo('multi_table', 'Bupa_v1')


Since we're in the process of fixing the demo metadata, we might want to update this to modify the downloaded demo so that it is guaranteed to be invalid

frances-h · 2024-03-29T15:16:00Z

sdv/multi_table/utils.py

+    root_tables = _get_root_tables(relationships)
+    all_descendants = {}
+    for root in root_tables:
+        order_descendances = _get_n_order_descendants(relationships, root, order)


minor: typo

codecov-commenter · 2024-03-29T16:04:45Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.55%. Comparing base (27d1b25) to head (209b720).
Report is 3 commits behind head on main.

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1874      +/-   ##
==========================================
+ Coverage   97.46%   97.55%   +0.08%     
==========================================
  Files          51       52       +1     
  Lines        4978     5143     +165     
==========================================
+ Hits         4852     5017     +165     
  Misses        126      126

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

amontanez24 · 2024-04-01T22:13:44Z

sdv/multi_table/utils.py

+
+    Returns:
+        int:
+            Number of columns to drop.


it looks like this returns a tuple

amontanez24 · 2024-04-01T22:17:42Z

tests/unit/utils/test_poc.py

+def test_simplify_schema_invalid_data():
+    """Test ``simplify_schema`` when the data is not valid."""
+    # Setup
+    real_data, metadata = download_demo('multi_table', 'fake_hotels')


Ideally we shouldn't download data for the unit tests

sdv/multi_table/utils.py

R-Palazzo · 2024-04-02T13:26:09Z

Thanks for the review @amontanez24, I addressed latest comments in this commit 8ca4158.

amontanez24 · 2024-04-03T19:04:12Z

sdv/multi_table/utils.py

+def _get_relationship_for_child(relationships, child_table):
+    return [rel for rel in relationships if rel['child_table_name'] == child_table]
+
+
+def _get_relationship_for_parent(relationships, parent_table):


minor: can we make plural (ie. get_relationships)

Yes, done in 209b720

amontanez24 · 2024-04-03T19:12:00Z

sdv/multi_table/utils.py

+    for root in root_tables:
+        order_descendants = _get_n_order_descendants(relationships, root, order)
+        all_descendant_root = set()
+        for order_key in order_descendants:
+            all_descendant_root.update(order_descendants[order_key])
+
+        all_descendants[root] = all_descendant_root
+


could we prevent having to recompute children and grandchildren later by having this method return a dict in the following format:

root -> {'order_1': ..., 'order_2': ..., 'num_descendants': 6}

Good idea, done in 209b720

frances-h · 2024-04-04T15:23:11Z

sdv/multi_table/hma.py

-            self._estimate_columns_traversal(table_name, columns_per_table, visited)
-
-        return columns_per_table
+                "We recommend simplifying your metadata schema using 'sdv.utils.simplify_schema'."


minor: should be "using 'sdv.utils.poc.simplify_schema'"

sdv/multi_table/utils.py

frances-h · 2024-04-04T15:33:03Z

sdv/multi_table/utils.py

+            elif is_child_to_drop and child_table in metadata.tables:
+                del metadata.tables[child_table]
+
+    return metadata


Since this function will actually modify the metadata object we pass in, we should remove the return. The convention is usually to have functions that modify arguments in-place to not return anything.

frances-h · 2024-04-04T15:33:33Z

sdv/multi_table/utils.py

+        for column in columns_to_drop:
+            del columns[column]
+
+    return metadata


Same as above, we should remove the return since we're directly modifying the passed in metadata object

frances-h · 2024-04-04T15:43:15Z

sdv/multi_table/utils.py

+            - num_cols_to_drop (int): Number of columns to drop from the child table.
+            - modelable_columns (dict): Dictionary that maps the modelable sdtype to their columns.
+    """
+    num_column_parameter = 4  # for beta distribution


Could we pull this from HMA instead of using a magic number? Maybe update the distribution to number of parameters dict to a constant, and pull it and HMA's default distribution (in case it ever changes) directly from HMA?

frances-h · 2024-04-04T15:52:41Z

sdv/multi_table/utils.py

+    columns_to_sdtypes = {
+        column: columns[column]['sdtype'] for column in columns
+    }
+    sdtypes_to_columns = defaultdict(list)
+    for col, sdtype in columns_to_sdtypes.items():
+        sdtypes_to_columns[sdtype].append(col)
+
+    modelable_columns = {
+        key: value for key, value in sdtypes_to_columns.items() if key in MODELABLE_SDTYPE
+    }


I think this can be simplified to something like:

modelable_columns = defaultdict(list) for col, col_meta in columns.items(): if col_meta['sdtype'] in MODELABLE_SDTYPE: modelable_columns[col_meta['sdtype'].append(col)

R-Palazzo · 2024-04-05T08:48:01Z

Thanks @frances-h, @amontanez24 for the review. All comments should have been addressed.

amontanez24

LGTM🍾 🎆

frances-h

LGTM, thanks for addressing! 🎉

R-Palazzo requested review from amontanez24 and frances-h March 26, 2024 21:54

R-Palazzo requested a review from a team as a code owner March 26, 2024 21:54

R-Palazzo removed the request for review from a team March 26, 2024 21:54

amontanez24 requested changes Mar 28, 2024

View reviewed changes

frances-h reviewed Mar 29, 2024

View reviewed changes

R-Palazzo requested review from amontanez24 and frances-h April 1, 2024 10:15

amontanez24 reviewed Apr 1, 2024

View reviewed changes

R-Palazzo force-pushed the issue-1832-simplify-schema branch from 5d72c6a to 8ca4158 Compare April 2, 2024 13:23

R-Palazzo requested a review from amontanez24 April 2, 2024 13:24

R-Palazzo added 15 commits April 3, 2024 11:32

first def _utils

b5c3462

move function fro hsa

8ee169a

define utils mutli table

01162db

first def poc

872737c

update hma to have static and class method

60333bb

unit tests poc

becf8b5

define MAX_NUMBER_OF_COLUMNS

665f9a3

unit tests utils

4ce1751

integration tests poc

18abf37

update warning message

eee1516

add _print_simplified_schema_summary

eebac5d

lint

831d70f

fix test minimum version

c06deea

add data/metadata validation

eb25e7a

typo + docstring + naming

8d5a501

R-Palazzo added 8 commits April 3, 2024 11:36

use only random + test + descendants

e55c2fe

remove one _get_root_tables from hma

0ef3abf

switch integration to unit for coverage

6f77ba2

simplified logic

5d47cc6

add unit test get_distributions

6f5e5cc

typo

535a949

address comments

955776e

docstring

eab810d

R-Palazzo force-pushed the issue-1832-simplify-schema branch from a7cf99b to eab810d Compare April 3, 2024 10:47

typo

8864b8e

amontanez24 reviewed Apr 3, 2024

View reviewed changes

address comments

209b720

R-Palazzo requested a review from amontanez24 April 4, 2024 12:27

frances-h reviewed Apr 4, 2024

View reviewed changes

R-Palazzo added 2 commits April 4, 2024 19:17

returns + docstring + message

2631cbd

HMA num parameter column + default dict

e32127c

R-Palazzo requested a review from frances-h April 5, 2024 08:48

amontanez24 approved these changes Apr 5, 2024

View reviewed changes

frances-h approved these changes Apr 5, 2024

View reviewed changes

R-Palazzo merged commit fe72d91 into main Apr 5, 2024
37 checks passed

R-Palazzo deleted the issue-1832-simplify-schema branch April 5, 2024 18:15

R-Palazzo restored the issue-1832-simplify-schema branch April 5, 2024 19:33

R-Palazzo deleted the issue-1832-simplify-schema branch April 5, 2024 19:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add utility function to simplify multi-table schemas #1874

Add utility function to simplify multi-table schemas #1874

R-Palazzo commented Mar 26, 2024 •

edited

Loading

sdv-team commented Mar 26, 2024

amontanez24 left a comment

amontanez24 Mar 28, 2024

frances-h Mar 28, 2024

frances-h Mar 29, 2024

codecov-commenter commented Mar 29, 2024 •

edited

Loading

amontanez24 Apr 1, 2024

amontanez24 Apr 1, 2024

R-Palazzo commented Apr 2, 2024

amontanez24 Apr 3, 2024

R-Palazzo Apr 4, 2024

amontanez24 Apr 3, 2024

R-Palazzo Apr 4, 2024

frances-h Apr 4, 2024

frances-h Apr 4, 2024

frances-h Apr 4, 2024

frances-h Apr 4, 2024

frances-h Apr 4, 2024

R-Palazzo commented Apr 5, 2024

amontanez24 left a comment

frances-h left a comment

Add utility function to simplify multi-table schemas #1874

Add utility function to simplify multi-table schemas #1874

Conversation

R-Palazzo commented Mar 26, 2024 • edited Loading

sdv-team commented Mar 26, 2024

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

codecov-commenter commented Mar 29, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

R-Palazzo commented Apr 2, 2024

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

R-Palazzo commented Apr 5, 2024

amontanez24 left a comment

Choose a reason for hiding this comment

frances-h left a comment

Choose a reason for hiding this comment

R-Palazzo commented Mar 26, 2024 •

edited

Loading

codecov-commenter commented Mar 29, 2024 •

edited

Loading