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.

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

Use curies.Converter more natively #401

Closed
wants to merge 23 commits into from
Closed

Use curies.Converter more natively #401

wants to merge 23 commits into from

Conversation

cthoyt
Copy link
Member

@cthoyt cthoyt commented Jul 28, 2023

Blocked by #418

This PR is the next step towards using curies.Converter.

  • Replace the prefix_map inside the Metadata definition with a converter. Next steps are to do this for all models.
  • Throw away many duplicate implementations of prefix map merging and replace with curies.chain as the one true implementation

src/sssom/context.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@matentzn matentzn left a comment

Choose a reason for hiding this comment

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

Some quick comments, didnt do a thorough review yet.

There wont be any reason to distinguish internal and external converters moving forward, because we update the EPM during release and can ensure consistency with QC.

src/sssom/context.py Outdated Show resolved Hide resolved
src/sssom/context.py Outdated Show resolved Hide resolved
src/sssom/context.py Outdated Show resolved Hide resolved
src/sssom/context.py Outdated Show resolved Hide resolved
src/sssom/context.py Outdated Show resolved Hide resolved
src/sssom/context.py Outdated Show resolved Hide resolved
src/sssom/context.py Outdated Show resolved Hide resolved
)
return prefix_map
converter = Converter.from_prefix_map(prefix_map)
return curies.chain([converter, get_built_in_converter()])
Copy link
Collaborator

Choose a reason for hiding this comment

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

cool this is possible but not needed anymore: we just use the bioregistry converter and make sure through unit testing that it never violates our prefix preferences when updating the EPM.

src/sssom/io.py Outdated Show resolved Hide resolved
@@ -556,7 +561,7 @@ def from_sssom_rdf(

ms.mappings = mlist # type: ignore
_set_metadata_in_mapping_set(mapping_set=ms, metadata=meta)
mdoc = MappingSetDocument(mapping_set=ms, prefix_map=prefix_map)
mdoc = MappingSetDocument(mapping_set=ms, prefix_map=converter.prefix_map)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Now I get it. Great.

Copy link
Member Author

Choose a reason for hiding this comment

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

Next step will be replace the prefix_map object in the mapping set document with a converter directly

@matentzn matentzn mentioned this pull request Aug 5, 2023
1 task
@matentzn
Copy link
Collaborator

matentzn commented Aug 7, 2023

Lost track of this. @cthoyt I know you will be swamped, so let me know when this is ready for review.

@cthoyt cthoyt deleted the epms-for-all branch September 26, 2023 14:47
@cthoyt
Copy link
Member Author

cthoyt commented Sep 26, 2023

This has been split into many other PRs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants