Repository: mapping-commons/sssom-py

Issue:
Mapping set inversion fails if the set contains a `subject_*` column but no corresponding `object_*` column
Trying to `sssom invert` a mapping set that contains a `subject_label` column but no `object_label` yields an error because `sssom-py` somehow expects to find a `subject_label` column in the inverted set, even though the inverted set will (logically) only contain a `object_label` column.

Example: given the following minimalist set:

```
#curie_map:
#  COMENT: https://example.com/entities/
#  ORGENT: https://example.org/entities/
#mapping_set_id: https://example.org/sets/exo2c
subject_id      subject_label  predicate_id    object_id       mapping_justification
ORGENT:0001     alice      skos:closeMatch COMENT:0011     semapv:ManualMappingCuration
```

the following command:

```sh
sssom-dev invert sample.sssom.tsv -o inverted.sssom.tsv
```

yields the following error:

```
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/home/damien/.cache/pypoetry/virtualenvs/sssom-tFe7wmy3-py3.9/lib64/python3.9/site-packages/click/core.py", line 1157, in __call__
    return self.main(*args, **kwargs)
  File "/home/damien/.cache/pypoetry/virtualenvs/sssom-tFe7wmy3-py3.9/lib64/python3.9/site-packages/click/core.py", line 1078, in main
    rv = self.invoke(ctx)
  File "/home/damien/.cache/pypoetry/virtualenvs/sssom-tFe7wmy3-py3.9/lib64/python3.9/site-packages/click/core.py", line 1688, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/home/damien/.cache/pypoetry/virtualenvs/sssom-tFe7wmy3-py3.9/lib64/python3.9/site-packages/click/core.py", line 1434, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/home/damien/.cache/pypoetry/virtualenvs/sssom-tFe7wmy3-py3.9/lib64/python3.9/site-packages/click/core.py", line 783, in invoke
    return __callback(*args, **kwargs)
  File "/home/damien/dvlpt/semweb/python/sssom-py/src/sssom/cli.py", line 769, in invert
    msdf.df = invert_mappings(
  File "/home/damien/dvlpt/semweb/python/sssom-py/src/sssom/util.py", line 1467, in invert_mappings
    inverted_df = inverted_df[df.columns]
  File "/home/damien/.cache/pypoetry/virtualenvs/sssom-tFe7wmy3-py3.9/lib64/python3.9/site-packages/pandas/core/frame.py", line 3767, in __getitem__
    indexer = self.columns._get_indexer_strict(key, "columns")[1]
  File "/home/damien/.cache/pypoetry/virtualenvs/sssom-tFe7wmy3-py3.9/lib64/python3.9/site-packages/pandas/core/indexes/base.py", line 5877, in _get_indexer_strict
    self._raise_if_missing(keyarr, indexer, axis_name)
  File "/home/damien/.cache/pypoetry/virtualenvs/sssom-tFe7wmy3-py3.9/lib64/python3.9/site-packages/pandas/core/indexes/base.py", line 5941, in _raise_if_missing
    raise KeyError(f"{not_found} not in index")
KeyError: "['subject_label'] not in index"
```

More generally, it seems that the error is triggered by any `subject_*` column that does not have its `object_*` counterpart in the set. For example, replacing `subject_label` by `subject_source` in the example above will yield exactly the same error trace, with a `KeyError: "['subject_source'] not in index"` message.

This issue affects the conversion to OWL as well (`sssom convert -O owl`), because that conversion involves at some point the inversion of the mapping set to convert.

Issue originally found in https://github.com/monarch-initiative/omim/issues/114. Reproduced with the latest code from the master branch of `sssom-py`.

Discussion:
@gouttegd: I believe the problem lies [here](https://github.com/mapping-commons/sssom-py/blob/40b6fc50727dbeefdd4a5d7566efd02fdcec0e1a/src/sssom/util.py#L1467):

```python
inverted_df = df_to_invert.rename(
    columns=_invert_column_names(list_of_subject_object_columns, columns_invert_map)
)
inverted_df = inverted_df[df.columns]
```

As I understand it, the last line in that fragment is intended to reorder the columns in the `inverted_df`, so that they are in the same order as in the original `df_to_invert` despite their renaming. That is, the renaming turned, for example, `subject_id` into `object_id` and the other way around, but the columns are still in their original positions, so the reordering is necessary to ensure the renamed columns are at their expected positions (e.g., the new `subject_id` should be the first column).

But that reordering necessarily supposes that the inverted data frame will always contain the same columns as the original data frame. This is an unwarranted assumption. It won’t be the case if the set contains an `subject_*` column that does not have an `object_*` counterpart (which is perfectly valid in SSSOM, _except_ for `subject_id` and `object_id` which must both always be present).

Suggested fix:

```diff
-    inverted_df = inverted_df[df.columns]
+    inverted_df = sort_df_rows_columns(inverted_df, by_rows=False)
```

so that reordering is performed by the appropriate function.@matentzn: Yes, I think you are right! Thanks for drilling down and figuring it out.. With great tears we just lost our beloved primary software dev @hrshdhgd from Monarchs SSSOM team, so any additional help is very much welcome.@gouttegd: Fixed with #555 .@gouttegd: I think this would warrant a 0.4.12 bug fix release. The bug prevents `sssom invert` and `sssom convert -O owl` from working at all whenever the input set contains “imbalanced” `subject_*` and `object_*` columns, which is quite frequent -- in fact most of the mapping sets I have seen in the wild are in that case.@matentzn: I was going to do this and https://github.com/mapping-commons/sssom-py/pull/556 in one sweep - you think I should release two separate versions?@gouttegd: No, no need for two releases at short interval, I think. It can wait until #556 is merged. :)@matentzn: Release is out: https://pypi.org/project/sssom/0.4.12/
@gouttegd: Reopening as the issue has _not_ been fixed for all cases:

```sh
$ sssom invert wtf.sssom.tsv -o inverted-wtf.sssom.tsv
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/Users/dpg44/Library/Caches/pypoetry/virtualenvs/sssom-LAZDtc9M-py3.11/lib/python3.11/site-packages/click/core.py", line 1442, in __call__
    return self.main(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/dpg44/Library/Caches/pypoetry/virtualenvs/sssom-LAZDtc9M-py3.11/lib/python3.11/site-packages/click/core.py", line 1363, in main
    rv = self.invoke(ctx)
         ^^^^^^^^^^^^^^^^
  File "/Users/dpg44/Library/Caches/pypoetry/virtualenvs/sssom-LAZDtc9M-py3.11/lib/python3.11/site-packages/click/core.py", line 1830, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/dpg44/Library/Caches/pypoetry/virtualenvs/sssom-LAZDtc9M-py3.11/lib/python3.11/site-packages/click/core.py", line 1226, in invoke
    return ctx.invoke(self.callback, **ctx.params)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/dpg44/Library/Caches/pypoetry/virtualenvs/sssom-LAZDtc9M-py3.11/lib/python3.11/site-packages/click/core.py", line 794, in invoke
    return callback(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/dpg44/Development/PySemOnt/sssom-py/src/sssom/cli.py", line 773, in invert
    msdf.df = invert_mappings(
              ^^^^^^^^^^^^^^^^
  File "/Users/dpg44/Development/PySemOnt/sssom-py/src/sssom/util.py", line 1456, in invert_mappings
    columns=_invert_column_names(list_of_subject_object_columns, columns_invert_map)
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/dpg44/Development/PySemOnt/sssom-py/src/sssom/util.py", line 1476, in _invert_column_names
    return {x: columns_invert_map[x] for x in column_names}
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/dpg44/Development/PySemOnt/sssom-py/src/sssom/util.py", line 1476, in <dictcomp>
    return {x: columns_invert_map[x] for x in column_names}
               ~~~~~~~~~~~~~~~~~~^^^
KeyError: 'subject_type'
```

where `wtf.sssom.tsv` is a set containing mappings with a `subject_type` slot but no `object_type` slot:

```
#curie_map:
#  FOODON: http://purl.obolibrary.org/obo/FOODON_
#  KF_FOOD: https://kewl-foodie.inc/food/
#mapping_set_id: https://example.org/sample-set
#license: https://creativecommons.org/licenses/by/4.0/
subject_id	predicate_id	object_id	mapping_justification	subject_type
KF_FOOD:F001	skos:exactMatch	FOODON:00002473	semapv:ManualMappingCuration	owl class
```

Solution patch:
diff --git a/src/sssom/util.py b/src/sssom/util.py
index fcd2912..f506c44 100644
--- a/src/sssom/util.py
+++ b/src/sssom/util.py
@@ -1464,7 +1464,7 @@ def invert_mappings(
     inverted_df = df_to_invert.rename(
         columns=_invert_column_names(list_of_subject_object_columns, columns_invert_map)
     )
-    inverted_df = inverted_df[df.columns]
+    inverted_df = sort_df_rows_columns(inverted_df, by_rows=False)
     inverted_df[PREDICATE_ID] = inverted_df[PREDICATE_ID].map(predicate_invert_map)
     if update_justification:
         inverted_df[MAPPING_JUSTIFICATION] = SEMAPV.MappingInversion.value


Test patch:
diff --git a/tests/data/asymmetric.tsv b/tests/data/asymmetric.tsv
new file mode 100644
index 0000000..5b44053
--- /dev/null
+++ b/tests/data/asymmetric.tsv
@@ -0,0 +1,17 @@
+#curie_map:
+#  orcid: https://orcid.org/
+#  x: http://example.org/x/
+#  y: http://example.org/y/
+#  z: http://example.org/z/
+#mapping_set_id: https://w3id.org/sssom/mapping/tests/data/asymmetric.tsv
+#creator_id:
+#  - orcid:1234
+#  - orcid:5678
+#license: https://creativecommons.org/publicdomain/zero/1.0/
+#mapping_date: 2020-05-30
+subject_id	subject_label	predicate_id	object_id	mapping_justification	object_source	confidence
+x:appendage	appendage	owl:equivalentClass	y:appendage	semapv:ManualMappingCuration	y:example	0.841
+x:appendage	appendage	owl:equivalentClass	z:appendage	semapv:LexicalMatching	z:example	0.882
+x:appendage	appendage	owl:equivalentClass	z:appendage	semapv:ManualMappingCuration	z:example	0.841
+x:bone_element	bone element	owl:equivalentClass	y:bone	semapv:LexicalMatching	y:example	0.739
+x:bone_element	bone element	owl:equivalentClass	y:bone	semapv:ManualMappingCuration	y:example	0.535
diff --git a/tests/test_utils.py b/tests/test_utils.py
index a6e1c80..54acee4 100644
--- a/tests/test_utils.py
+++ b/tests/test_utils.py
@@ -175,6 +175,15 @@ class TestIO(unittest.TestCase):
         inverted_df = invert_mappings(df=self.msdf2.df, merge_inverted=False)
         self.assertEqual(len(inverted_df), len(self.msdf2.df.drop_duplicates()))
 
+    def test_invert_asymmetric_nodes(self):
+        """Test inverting sets containing imbalanced subject/object columns."""
+        msdf = parse_sssom_table(f"{data_dir}/asymmetric.tsv")
+        inverted_df = invert_mappings(msdf.df, merge_inverted=False)
+        self.assertEqual(len(inverted_df), len(msdf.df))
+        original_subject_labels = msdf.df["subject_label"].values
+        inverted_object_labels = inverted_df["object_label"].values
+        self.assertNotIn(False, original_subject_labels == inverted_object_labels)
+
     def test_inject_metadata_into_df(self):
         """Test injecting metadata into DataFrame is as expected."""
         expected_creators = "orcid:0000-0001-5839-2535|orcid:0000-0001-5839-2532"