Drop Dataset.relations and make Dataset data-path-aware

Author: popescu-v

khiops/sklearn/dataset.py

@@ -176,7 +176,7 @@ def _check_multitable_spec(ds_spec):
             )
 
 
-def _table_name_of_path(table_path):
+def table_name_of_path(table_path):
     return table_path.split("/")[-1]
 
 
@@ -387,7 +387,6 @@ def __init__(self, X, y=None, categorical_target=True):
         # Initialize members
         self.main_table = None
         self.additional_data_tables = None
-        self.relations = None
         self.categorical_target = categorical_target
         self.target_column = None
         self.target_column_id = None
@@ -437,7 +436,8 @@ def __init__(self, X, y=None, categorical_target=True):
         # Index the tables by name
         self._tables_by_name = {
             table.name: table
-            for table in [self.main_table] + self.additional_data_tables
+            for table in [self.main_table]
+            + [table for _, table, _ in self.additional_data_tables]
         }
 
         # Post-conditions
@@ -513,32 +513,21 @@ def _init_tables_from_mapping(self, X):
                 key=main_table_key,
             )
             self.additional_data_tables = []
-            self.relations = []
             if "additional_data_tables" in X:
                 for table_path, table_spec in X["additional_data_tables"].items():
                     table_source, table_key = table_spec[:2]
-                    table_name = _table_name_of_path(table_path)
+                    table_name = table_name_of_path(table_path)
                     table = PandasTable(
                         table_name,
                         table_source,
-                        data_path=table_path,
                         key=table_key,
                     )
-                    self.additional_data_tables.append(table)
                     is_one_to_one_relation = False
                     if len(table_spec) == 3 and table_spec[2] is True:
                         is_one_to_one_relation = True
 
-                    # Set relation parent: if no "/" in path, main_table is the parent
-                    if not "/" in table_path:
-                        parent_table_name = self.main_table.name
-                    else:
-                        table_path_fragments = table_path.split("/")
-                        parent_table_name = _table_name_of_path(
-                            "/".join(table_path_fragments[:-1])
-                        )
-                    self.relations.append(
-                        (parent_table_name, table_name, is_one_to_one_relation)
+                    self.additional_data_tables.append(
+                        (table_path, table, is_one_to_one_relation)
                     )
         # Initialize a sparse dataset (monotable)
         elif isinstance(main_table_source, sp.spmatrix):
@@ -548,7 +537,6 @@ def _init_tables_from_mapping(self, X):
                 key=main_table_key,
             )
             self.additional_data_tables = []
-            self.relations = []
         # Initialize a numpyarray dataset (monotable)
         elif hasattr(main_table_source, "__array__"):
             self.main_table = NumpyTable(
@@ -561,7 +549,6 @@ def _init_tables_from_mapping(self, X):
                     "with pandas dataframe source tables"
                 )
             self.additional_data_tables = []
-            self.relations = []
         else:
             raise TypeError(
                 type_error_message(
@@ -680,11 +667,12 @@ def to_spec(self):
         ds_spec = {}
         ds_spec["main_table"] = (self.main_table.data_source, self.main_table.key)
         ds_spec["additional_data_tables"] = {}
-        for table in self.additional_data_tables:
-            assert table.data_path is not None
-            ds_spec["additional_data_tables"][table.data_path] = (
+        for table_path, table, is_one_to_one_relation in self.additional_data_tables:
+            assert table_path is not None
+            ds_spec["additional_data_tables"][table_path] = (
                 table.data_source,
                 table.key,
+                is_one_to_one_relation,
             )
 
         return ds_spec
@@ -748,31 +736,32 @@ def create_khiops_dictionary_domain(self):
         # Note: In general 'name' and 'object_type' fields of Variable can be different
         if self.additional_data_tables:
             main_dictionary.root = True
-            table_names = [table.name for table in self.additional_data_tables]
-            tables_to_visit = [self.main_table.name]
-            while tables_to_visit:
-                current_table = tables_to_visit.pop(0)
-                for relation in self.relations:
-                    parent_table, child_table, is_one_to_one_relation = relation
-                    if parent_table == current_table:
-                        tables_to_visit.append(child_table)
-                        parent_table_name = parent_table
-                        index_table = table_names.index(child_table)
-                        table = self.additional_data_tables[index_table]
-                        parent_table_dictionary = dictionary_domain.get_dictionary(
-                            parent_table_name
-                        )
-                        dictionary = table.create_khiops_dictionary()
-                        dictionary_domain.add_dictionary(dictionary)
-                        table_variable = kh.Variable()
-                        if is_one_to_one_relation:
-                            table_variable.type = "Entity"
-                        else:
-                            table_variable.type = "Table"
-                        table_variable.name = table.name
-                        table_variable.object_type = table.name
-                        parent_table_dictionary.add_variable(table_variable)
+            for (
+                table_path,
+                table,
+                is_one_to_one_relation,
+            ) in self.additional_data_tables:
+                if not "/" in table_path:
+                    parent_table_name = self.main_table.name
+                else:
+                    table_path_fragments = table_path.split("/")
+                    parent_table_name = table_name_of_path(
+                        "/".join(table_path_fragments[:-1])
+                    )
+                parent_table_dictionary = dictionary_domain.get_dictionary(
+                    parent_table_name
+                )
 
+                dictionary = table.create_khiops_dictionary()
+                dictionary_domain.add_dictionary(dictionary)
+                table_variable = kh.Variable()
+                if is_one_to_one_relation:
+                    table_variable.type = "Entity"
+                else:
+                    table_variable.type = "Table"
+                table_variable.name = table.name
+                table_variable.object_type = table.name
+                parent_table_dictionary.add_variable(table_variable)
         return dictionary_domain
 
     def create_table_files_for_khiops(self, output_dir, sort=True):
@@ -811,9 +800,9 @@ def create_table_files_for_khiops(self, output_dir, sort=True):
 
         # Create a copy of each secondary table
         secondary_table_paths = {}
-        for table in self.additional_data_tables:
-            assert table.data_path is not None
-            secondary_table_paths[table.data_path] = table.create_table_file_for_khiops(
+        for table_path, table, _ in self.additional_data_tables:
+            assert table_path is not None
+            secondary_table_paths[table_path] = table.create_table_file_for_khiops(
                 output_dir, sort=sort
             )
 
@@ -918,13 +907,11 @@ class PandasTable(DatasetTable):
         Name for the table.
     dataframe : `pandas.DataFrame`
         The data frame to be encapsulated. It must be non-empty.
-    data_path : str, optional
-        Data path of the table. Unset for main tables.
     key : list of str, optional
         The names of the columns composing the key.
     """
 
-    def __init__(self, name, dataframe, data_path=None, key=None):
+    def __init__(self, name, dataframe, key=None):
         # Call the parent method
         super().__init__(name=name, key=key)
 
@@ -937,7 +924,6 @@ def __init__(self, name, dataframe, data_path=None, key=None):
         # Initialize the attributes
         self.data_source = dataframe
         self.n_samples = len(self.data_source)
-        self.data_path = data_path
 
         # Initialize feature columns and verify their types
         self.column_ids = self.data_source.columns.values

khiops/sklearn/estimators.py

@@ -1516,7 +1516,7 @@ def _transform_check_dataset(self, ds):
 
         # Multi-table model: Check name and dictionary coherence of secondary tables
         dataset_secondary_tables_by_name = {
-            table.name: table for table in ds.additional_data_tables
+            table.name: table for _, table, _ in ds.additional_data_tables
         }
         for dictionary in self.model_.dictionaries:
             assert dictionary.name.startswith(self._khiops_model_prefix), (

khiops/sklearn/helpers.py

@@ -11,7 +11,7 @@
 from sklearn.model_selection import train_test_split
 
 from khiops.core.internals.common import is_dict_like, type_error_message
-from khiops.sklearn.dataset import Dataset
+from khiops.sklearn.dataset import Dataset, table_name_of_path
 
 # Note: We build the splits with lists and itertools.chain avoid pylint warning about
 # unbalanced-tuple-unpacking. See issue https://github.com/pylint-dev/pylint/issues/5671
@@ -122,15 +122,31 @@ def _train_test_split_in_memory_dataset(ds, y, test_size, sklearn_split_params=N
 
     # Split the secondary tables tables
     # Note: The tables are traversed in BFS
-    todo_relations = [
-        relation for relation in ds.relations if relation[0] == ds.main_table.name
+    todo_tables = [
+        (table_path, table)
+        for table_path, table, _ in ds.additional_data_tables
+        if "/" not in table_path
     ]
-    while todo_relations:
-        current_parent_table_name, current_child_table_name, _ = todo_relations.pop(0)
-        for relation in ds.relations:
-            parent_table_name, _, _ = relation
+    while todo_tables:
+        current_table_path, current_table = todo_tables.pop(0)
+        if "/" not in current_table_path:
+            current_parent_table_name = ds.main_table.name
+        else:
+            table_path_fragments = current_table_path.split("/")
+            current_parent_table_name = table_name_of_path(
+                "/".join(table_path_fragments[:-1])
+            )
+        current_child_table_name = current_table.name
+        for secondary_table_path, secondary_table, _ in ds.additional_data_tables:
+            if "/" not in secondary_table_path:
+                parent_table_name = ds.main_table.name
+            else:
+                table_path_fragments = secondary_table_path.split("/")
+                parent_table_name = table_name_of_path(
+                    "/".join(table_path_fragments[:-1])
+                )
             if parent_table_name == current_child_table_name:
-                todo_relations.append(relation)
+                todo_tables.append((secondary_table_path, secondary_table))
 
         for new_ds in (train_ds, test_ds):
             origin_child_table = ds.get_table(current_child_table_name)

tests/test_dataset_class.py

@@ -459,19 +459,22 @@ def test_dataset_is_correctly_built(self):
         self.assertEqual(dataset.main_table.name, "main_table")
         self.assertEqual(len(dataset.additional_data_tables), 4)
         dataset_secondary_table_names = {
-            secondary_table.name for secondary_table in dataset.additional_data_tables
+            secondary_table.name
+            for _, secondary_table, _ in dataset.additional_data_tables
         }
         self.assertEqual(dataset_secondary_table_names, {"B", "C", "D", "E"})
-        self.assertEqual(len(dataset.relations), 4)
 
         table_specs = ds_spec["additional_data_tables"].items()
-        for relation, (table_path, table_spec) in zip(dataset.relations, table_specs):
+        for (ds_table_path, _, ds_is_one_to_one), (
+            table_path,
+            table_spec,
+        ) in zip(dataset.additional_data_tables, table_specs):
             # The relation holds the table name, not the table path
-            self.assertEqual(relation[1], table_path.split("/")[-1])
+            self.assertEqual(ds_table_path, table_path)
             if len(table_spec) == 3:
-                self.assertEqual(relation[2], table_spec[2])
+                self.assertEqual(ds_is_one_to_one, table_spec[2])
             else:
-                self.assertFalse(relation[2])
+                self.assertFalse(ds_is_one_to_one)
 
     def test_out_file_from_dataframe_monotable(self):
         """Test consistency of the created data file with the input dataframe
@@ -745,7 +748,9 @@ def _test_domain_coherence(self, ds, ref_var_types):
 
         # Check that the domain has the same table names as the reference
         ref_table_names = {
-            table.name for table in [ds.main_table] + ds.additional_data_tables
+            table.name
+            for table in [ds.main_table]
+            + [table for _, table, _ in ds.additional_data_tables]
         }
         out_table_names = {dictionary.name for dictionary in out_domain.dictionaries}
         self.assertEqual(ref_table_names, out_table_names)
@@ -758,7 +763,9 @@ def _test_domain_coherence(self, ds, ref_var_types):
         # Check that:
         # - the table keys are the same as the dataset
         # - the domain has the same variable names as the reference
-        for table in [ds.main_table] + ds.additional_data_tables:
+        for table in [ds.main_table] + [
+            table for _, table, _ in ds.additional_data_tables
+        ]:
             with self.subTest(table=table.name):
                 self.assertEqual(table.key, out_domain.get_dictionary(table.name).key)
                 out_dictionary_var_types = {