search

mapping-commons / sssom-py

Python toolkit for SSSOM mapping format
https://mapping-commons.github.io/sssom-py/index.html#
MIT License
49 stars 12 forks source link

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

Closed joeflack4 closed 7 months ago

joeflack4 commented 7 months ago

Overview

This PR originally had more changes in order to provide fixes for https://github.com/monarch-initiative/mondo-ingest/pull/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.

matentzn commented 7 months ago

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

joeflack4 commented 7 months ago

@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 commented 7 months ago

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 commented 7 months ago

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

joeflack4 commented 7 months ago

@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: