Fix msdf_merge util function. (#627)

The `msdf_merge` function attempts to "propagate" slots before merging
the sets, but is doing so without any regard for which slots should
actually be propagated.

In addition, it attempts to inject in every individual mapping a
`mapping_set_source` slot pointing to the ID of the original set that
contained the mapping, but this is invalid as there is _no_
`mapping_set_source` slot on indivdual mapping records -- the slot
intended to capture the set from which a record came from is
`mapping_source`.

Lastly, the function also attempts to drop duplicates after the sets
have been merged, but the detection of duplicates is prevented by (1)
the incorrect propagation of non-propagatable slots (which can cause two
otherwise identical records in two different sets to appear different,
if the metadata of the sets contain different wrongly propagated slots),
and (2) the injection of the `mapping_set_source` slot.

This commit fixes all those issues by deleting the bogus
`inject_metadata_into_df` function and replacing by a call to
`msdf.propagate()`, which implements propagation correctly. It then
manually inject the correct `mapping_source` slot if possible, and if so
ignore the injected slot when attempting to drop duplicates.

Author: gouttegd

src/sssom/util.py

@@ -47,7 +47,7 @@
     MAPPING_CARDINALITY,
     MAPPING_JUSTIFICATION,
     MAPPING_SET_ID,
-    MAPPING_SET_SOURCE,
+    MAPPING_SOURCE,
     NO_TERM_FOUND,
     OBJECT_CATEGORY,
     OBJECT_ID,
@@ -1118,25 +1118,35 @@ def merge_msdf(
     :param reconcile: If reconcile=True, then dedupe(remove redundant lower confidence mappings) and
         reconcile (if msdf contains a higher confidence _negative_ mapping, then remove lower
         confidence positive one. If confidence is the same, prefer HumanCurated. If both
-        HumanCurated, prefer negative mapping). Defaults to True.
+        HumanCurated, prefer negative mapping). Defaults to False.
 
     :returns: Merged MappingSetDataFrame.
     """
-    # Inject metadata of msdf into df
-    msdf_with_meta = [inject_metadata_into_df(msdf) for msdf in msdfs]
-
-    # merge df [# 'outer' join in pandas == FULL JOIN in SQL]
-    # df_merged = reduce(
-    #     lambda left, right: left.merge(right, how="outer", on=list(left.columns)),
-    #     [msdf.df for msdf in msdf_with_meta],
-    # )
-    # Concat is an alternative to merge when columns are not the same.
+    # Propagate slots, inject source if possible
+    source_injected = 0
+    for msdf in msdfs:
+        msdf.propagate()
+        if MAPPING_SET_ID in msdf.metadata and MAPPING_SOURCE not in msdf.df.columns:
+            msdf.df[MAPPING_SOURCE] = msdf.metadata[MAPPING_SET_ID]
+            source_injected += 1
+
+    columns = set([c for msdf in msdfs for c in msdf.df.columns])
+    if source_injected > 1:
+        # If we injected a mapping_source slot into each individual
+        # record for at least two of the input sets, then we must ignore
+        # that slot when attempting to remove duplicates below, because
+        # the mere presence of that slot would cause two identical
+        # records to appear different just because they come from
+        # different sources (which they would not do if we had not
+        # injected the mapping_source above).
+        columns.remove(MAPPING_SOURCE)
+
     df_merged = reduce(
         lambda left, right: pd.concat([left, right], axis=0, ignore_index=True),
-        [msdf.df for msdf in msdf_with_meta],
-    ).drop_duplicates(ignore_index=True)
+        [msdf.df for msdf in msdfs],
+    ).drop_duplicates(ignore_index=True, subset=columns)
 
-    converter = curies.chain([msdf.converter for msdf in msdf_with_meta])
+    converter = curies.chain([msdf.converter for msdf in msdfs])
     merged_msdf = MappingSetDataFrame.with_converter(df=df_merged, converter=converter)
     if reconcile:
         merged_msdf.df = filter_redundant_rows(merged_msdf.df)
@@ -1296,27 +1306,6 @@ def deal_with_negation(df: pd.DataFrame) -> pd.DataFrame:
     return return_df
 
 
-def inject_metadata_into_df(msdf: MappingSetDataFrame) -> MappingSetDataFrame:
-    """Inject metadata dictionary key-value pair into DataFrame columns in a MappingSetDataFrame.DataFrame.
-
-    :param msdf: MappingSetDataFrame with metadata separate.
-
-    :returns: MappingSetDataFrame with metadata as columns
-    """
-    # TODO add this into the "standardize" function introduced in
-    #  https://github.com/mapping-commons/sssom-py/pull/438
-    # TODO Check if 'k' is a valid 'slot' for 'mapping' [sssom.yaml]
-    slots = SSSOMSchemaView().mapping_slots
-    for k, v in msdf.metadata.items():
-        if k not in msdf.df.columns and k in slots:
-            if k == MAPPING_SET_ID:
-                k = MAPPING_SET_SOURCE
-            if isinstance(v, list):
-                v = "|".join(x for x in v)
-            msdf.df[k] = str(v)
-    return msdf
-
-
 ExtensionLiteral = Literal["tsv", "csv"]
 
 

tests/test_merge.py

@@ -21,7 +21,7 @@ def setUp(self) -> None:
     def test_merge_multiple_inputs(self) -> None:
         """Test merging of multiple msdfs."""
         merged_msdf = merge_msdf(*self.msdfs)
-        self.assertEqual(275, len(merged_msdf.df))
+        self.assertEqual(200, len(merged_msdf.df))
 
     def test_merge_single_input(self) -> None:
         """Test merging when a single msdf is provided."""

tests/test_reconcile.py

@@ -41,7 +41,7 @@ def test_merge(self) -> None:
         msdf3 = parse_sssom_table(data_dir / "basic.tsv")
         merged_msdf1 = merge_msdf(self.msdf1, msdf3)
 
-        self.assertEqual(152, len(merged_msdf1.df))
+        self.assertEqual(149, len(merged_msdf1.df))
 
         merged_msdf2 = merge_msdf(self.msdf2, msdf3)
         self.assertEqual(174, len(merged_msdf2.df))

tests/test_utils.py

@@ -38,7 +38,6 @@
     get_dict_from_mapping,
     get_file_extension,
     get_prefixes_used_in_table,
-    inject_metadata_into_df,
     invert_mappings,
     is_multivalued_slot,
 )
@@ -213,14 +212,6 @@ def test_invert_asymmetric_nodes(self) -> None:
         inverted_object_labels = inverted_df["object_label"].values
         self.assertNotIn(False, original_subject_labels == inverted_object_labels)
 
-    def test_inject_metadata_into_df(self) -> None:
-        """Test injecting metadata into DataFrame is as expected."""
-        expected_creators = "orcid:0000-0001-5839-2535|orcid:0000-0001-5839-2532"
-        msdf = parse_sssom_table(f"{data_dir}/test_inject_metadata_msdf.tsv")
-        msdf_with_meta = inject_metadata_into_df(msdf)
-        creator_ids = msdf_with_meta.df["creator_id"].drop_duplicates().values.item()
-        self.assertEqual(creator_ids, expected_creators)
-
 
 class TestUtils(unittest.TestCase):
     """Unit test for utility functions."""