Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

lexical_index_to_sssom() EPM param #697

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

lexical_index_to_sssom() EPM param #697

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion src/oaklib/utilities/lexical/lexical_indexer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,7 @@ def lexical_index_to_sssom(
ruleset: MappingRuleCollection = None,
meta: Optional[Dict[str, t.Any]] = None,
prefix_map: Union[None, Converter, t.Mapping[str, str]] = None,
extended_prefix_map: t.Iterable[Dict[str, t.Any]] = None,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extended_prefix_map param added

I also wanted it to have the same type signature as in curies, which is LocationOr[Iterable[Union[Record, Dict[str, Any]]]] = None. However, when I did from curies.mapping_service.utils import Records, I got ModuleNotFoundError: No module named 'defusedxml'. I definitely don't think it's worth installing an additional package in OAK simply for better / more consistent typing.

I suppose an alternative to adding a new param would simply to overload the prefix_map param even more, but I opted not because itis already overloaded. I actually don't like this param because it supports either a prefix map Dict or a Converter object, and I think that makes it a little unclear / poorly named.

subjects: Collection[CURIE] = None,
objects: Collection[CURIE] = None,
symmetric: bool = False,
Expand All @@ -265,6 +266,9 @@ def lexical_index_to_sssom(
:param ensure_strict_prefixes: If true, prefixes & mappings in SSSOM MappingSetDataFrame will be filtred.
:return: SSSOM MappingSetDataFrame object.
"""
if prefix_map and extended_prefix_map:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exception handling

Throws error if both prefix_map and extended_prefix_map passed, as that creates ambiguity. The user should pick one.

raise ValueError("Can only pass prefix_map or extended_prefix_map, but not both.")

mappings = []
logging.info("Converting lexical index to SSSOM")
if subjects:
Expand Down Expand Up @@ -313,7 +317,11 @@ def lexical_index_to_sssom(

converter = curies.chain(
[
Converter.from_prefix_map((meta or {}).pop(CURIE_MAP, {})),
Converter.from_extended_prefix_map(extended_prefix_map)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How the param is used

This is handles the various scenarios in which a user might pass a prefix map. Before my changes there were already 2 ways in which a user might pass it, either within meta or the prefix_map args. There may be some redundancy here with ensure_converter() that I may not have fully considered.

if extended_prefix_map
else Converter.from_prefix_map(prefix_map)
if prefix_map
else Converter.from_prefix_map((meta or {}).pop(CURIE_MAP, {})),
ensure_converter(prefix_map, use_defaults=False),
oi.converter,
]
Expand Down