Bugfix: `clean_prefix_map()` not detecting prefix synonyms #485

joeflack4 · 2024-01-13T23:43:16Z

Overview

This PR originally had more changes in order to provide fixes for monarch-initiative/mondo-ingest#394. However it is no longer necessary to merge this in order for that PR to be merged.

This PR is now just a bugfix in clean_prefix_map() in which the converter object contains prefix synonyms.

@twhetzel FYI for your review!
@souzadevinicius A lot going on here, but FYI. This is related to my mondo ingest pr.

src/sssom/io.py

src/sssom/util.py

src/sssom/io.py

Motivated by mapping-commons/sssom-py#485

src/sssom/io.py

- Bugfix: clean_prefix_map(): Was ignoring prefix aliases. This was causing an error, as there was a perceived mismatch between the prefixes of the mapping set, and the prefix_map. - Bugfix: get_metadata_and_prefix_map(): Was not utilizing extended prefix maps. This manifested in issue where prefix aliases were not incorporated. This meant that (a) if we tried to fix by removing the alias from the plain prefix_map, these CURIEs could not be resolved, (b) if we included the alias in the plain prefix_map, there would be a duplicate URI prefix, which would result in an error. - Add: convert_plain_prefix_map_to_extended()

Implement function to deterministically make an EPM from a non-bijective prefix map, motivated by mapping-commons/sssom-py#485

src/sssom/util.py

- Delete: convert_plain_prefix_map_to_extended() - Update: get_metadata_and_prefix_map(): Reverted changes several commits ago utilizing convert_plain_prefix_map_to_extended()

This is a small test that illustrates why we cant just use `include_synonyms=True` to avoid the "missing prefix error" here https://github.com/mapping-commons/sssom-py/pull/485/files#diff-9f0c9a59dd545560607697feafaa62deb84dfd657f868fbeed9275e2acc0d9a6R249

matentzn · 2024-01-20T17:57:49Z

src/sssom/util.py

@@ -246,7 +246,7 @@ def clean_prefix_map(self, strict: bool = True) -> None:
        if self.metadata:
            prefixes_in_table.update(get_prefixes_used_in_metadata(self.metadata))

-        missing_prefixes = prefixes_in_table - self.converter.get_prefixes()
+        missing_prefixes = prefixes_in_table - self.converter.get_prefixes(include_synonyms=True)


Maybe i am wrong. I added a test below that illustrates the issue.

Lets say the raw data (prefixes in table) contains snomed. The converter has SCTID as a prefix. snomed as a synonym. Now this line of code here will not include snomed in missing prefixes, and move on.

Now you go a few lines below to extract a subconverter over all the prefixes in the table, including snomed, which will now become the main converter (this part is not very relevant for the problem but bear with me). Now, when the prefixmap gets serialsed to a valid SSSOM file, which uses msdf.converter.bimap (see write_table() method). This map does not contain snomed. so it will write SCTID, while the data (the mappings) still contains snomed.

If you still think this PR is right, you need to convince me that giving a mapping set with a value containing snomed, and snomed being a synonym in the EPM, the serialised SSSOM files contains snomed prefix..

Mm, I see the problem. I think you touched on this at the last meeting. How do you feel about serializing a non-bijective prefix map which would include the synonyms, then?

When it comes to working with synonyms in SSSOM, do you think this we've covered all of the cases? Or are there likely to be more places where synonym coverage is needed?

I don't see an issue on the topic of "EPM" / "extended prefix map" or "synonyms". Should we close this for now and move the discussion to a new issue?

cthoyt · 2024-01-20T22:20:19Z

tests/test_utils.py

+
+        converter = Converter.from_extended_prefix_map(EPM)
+        self.assertNotIn("snomed", converter.get_prefixes(include_synonyms=False))
+        self.assertIn("snomed", converter.get_prefixes(include_synonyms=True))


it's not SSSOM-py's job to test curies functionality. The test should show how the change in this PR actually effects input and output to SSSOM-py functions

This is not a test, it's some code to show why the pr the way it is should not be merged.. I don't intend to add this as a test here

matentzn · 2024-02-04T10:40:27Z

@joeflack4 - I think this PR should be closed, unless you think there is something we should salvage.

joeflack4 · 2024-02-04T21:37:02Z

@matentzn This PR is now a 1 line change:
missing_prefixes = prefixes_in_table - self.converter.get_prefixes(include_synonyms=True)

And it would seem it is a bugfix. It seems like this is something that should be merged.

I suppose I can pick up where Charlie left off and make sure that the test passes. I haven't looked at it yet.

matentzn · 2024-02-05T07:48:38Z

I thought I had shown that there is a problem with this approach - check my comments and the "test" that illustrates the issue. Maybe my assessment is wrong, but then you have to proof that my test is wrong 😁

joeflack4 · 2024-02-05T23:32:50Z

@matentzn Ah I'm sorry you're right, I see your last discussion.

joeflack4 · 2024-02-05T23:33:19Z

@matentzn I closed it but we have something unresolved that I mentioned the last time:

I don't see an issue on the topic of "EPM" / "extended prefix map" or "synonyms". Should we close this for now and move the discussion to a new issue?

See:

Extended prefix map support #496

joeflack4 mentioned this pull request Jan 13, 2024

Bug in match-mondo-sources-all-lexical.py monarch-initiative/mondo-ingest#394

Closed

8 tasks

joeflack4 requested review from cthoyt, matentzn and hrshdhgd January 13, 2024 23:43

joeflack4 added the bug Something isn't working label Jan 13, 2024

joeflack4 self-assigned this Jan 13, 2024

joeflack4 commented Jan 13, 2024

View reviewed changes

src/sssom/io.py Outdated Show resolved Hide resolved

joeflack4 force-pushed the epm-updates branch from 695ecab to b4843e1 Compare January 13, 2024 23:56

joeflack4 commented Jan 14, 2024

View reviewed changes

src/sssom/util.py Outdated Show resolved Hide resolved

joeflack4 commented Jan 14, 2024

View reviewed changes

src/sssom/io.py Outdated Show resolved Hide resolved

joeflack4 force-pushed the epm-updates branch 2 times, most recently from 79d8716 to 157cef4 Compare January 14, 2024 00:36

cthoyt added a commit to biopragmatics/curies that referenced this pull request Jan 15, 2024

Add function for handling a bad prefix map

920b077

Motivated by mapping-commons/sssom-py#485

cthoyt mentioned this pull request Jan 15, 2024

Upgrading (non-bijective) prefix maps biopragmatics/curies#99

Merged

8 tasks

joeflack4 commented Jan 15, 2024

View reviewed changes

src/sssom/io.py Outdated Show resolved Hide resolved

joeflack4 force-pushed the epm-updates branch from 157cef4 to 764dc1e Compare January 16, 2024 01:27

joeflack4 mentioned this pull request Jan 16, 2024

Deprecate get_metadata_and_prefix_map #486

Merged

cthoyt added a commit to biopragmatics/curies that referenced this pull request Jan 16, 2024

Upgrading (non-bijective) prefix maps (#99)

faef0bd

Implement function to deterministically make an EPM from a non-bijective prefix map, motivated by mapping-commons/sssom-py#485

cthoyt added 3 commits January 16, 2024 11:03

Switch to curies impl

57dd171

Update poetry.lock

2a59f73

Clean up with new curies functionality

565dd0f

matentzn reviewed Jan 17, 2024

View reviewed changes

src/sssom/util.py Show resolved Hide resolved

joeflack4 mentioned this pull request Jan 20, 2024

lexical_index_to_sssom() EPM param INCATools/ontology-access-kit#697

Open

Reverted to essential EPM bugfix

6790d7d

- Delete: convert_plain_prefix_map_to_extended() - Update: get_metadata_and_prefix_map(): Reverted changes several commits ago utilizing convert_plain_prefix_map_to_extended()

joeflack4 changed the title ~~Extended prefix maps usage updates~~ Bugfix: clean_prefix_map() not detecting prefix synonyms Jan 20, 2024

matentzn reviewed Jan 20, 2024

View reviewed changes

cthoyt reviewed Jan 20, 2024

View reviewed changes

joeflack4 mentioned this pull request Jan 31, 2024

Extended prefix map monarch-initiative/mondo#7221

Open

matentzn marked this pull request as draft February 4, 2024 10:40

joeflack4 marked this pull request as ready for review February 4, 2024 21:35

joeflack4 closed this Feb 5, 2024

joeflack4 mentioned this pull request Feb 5, 2024

Extended prefix map support #496

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfix: `clean_prefix_map()` not detecting prefix synonyms #485

Bugfix: `clean_prefix_map()` not detecting prefix synonyms #485

joeflack4 commented Jan 13, 2024 •

edited

Loading

matentzn Jan 20, 2024

joeflack4 Jan 20, 2024 •

edited

Loading

cthoyt Jan 20, 2024

matentzn Jan 20, 2024

matentzn commented Feb 4, 2024

joeflack4 commented Feb 4, 2024

matentzn commented Feb 5, 2024

joeflack4 commented Feb 5, 2024

joeflack4 commented Feb 5, 2024 •

edited

Loading

Bugfix: clean_prefix_map() not detecting prefix synonyms #485

Bugfix: clean_prefix_map() not detecting prefix synonyms #485

Conversation

joeflack4 commented Jan 13, 2024 • edited Loading

Overview

matentzn Jan 20, 2024

Choose a reason for hiding this comment

joeflack4 Jan 20, 2024 • edited Loading

Choose a reason for hiding this comment

cthoyt Jan 20, 2024

Choose a reason for hiding this comment

matentzn Jan 20, 2024

Choose a reason for hiding this comment

matentzn commented Feb 4, 2024

joeflack4 commented Feb 4, 2024

matentzn commented Feb 5, 2024

joeflack4 commented Feb 5, 2024

joeflack4 commented Feb 5, 2024 • edited Loading

Bugfix: `clean_prefix_map()` not detecting prefix synonyms #485

Bugfix: `clean_prefix_map()` not detecting prefix synonyms #485

joeflack4 commented Jan 13, 2024 •

edited

Loading

joeflack4 Jan 20, 2024 •

edited

Loading

joeflack4 commented Feb 5, 2024 •

edited

Loading