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

Fix outdated prefix map #297

Merged
merged 6 commits into from
Aug 4, 2022
Merged

Fix outdated prefix map #297

merged 6 commits into from
Aug 4, 2022

Conversation

matentzn
Copy link
Collaborator

@matentzn matentzn commented Aug 3, 2022

@hrshdhgd can you fix this for me? It was still using the old prefix map..

@hrshdhgd
Copy link
Contributor

hrshdhgd commented Aug 3, 2022

I have the fix in place

self = <tests.test_parsers.TestParse testMethod=test_parse_obographs>
(https://github.com/mapping-commons/sssom-py/runs/7661877722?check_suite_focus=true#step:8:43)

(https://github.com/mapping-commons/sssom-py/runs/7661877722?check_suite_focus=true#step:8:44)
    def test_parse_obographs(self):
(https://github.com/mapping-commons/sssom-py/runs/7661877722?check_suite_focus=true#step:8:45)
        """Test parsing OBO Graph JSON."""
(https://github.com/mapping-commons/sssom-py/runs/7661877722?check_suite_focus=true#step:8:46)
        msdf = from_obographs(
(https://github.com/mapping-commons/sssom-py/runs/7661877722?check_suite_focus=true#step:8:47)
            jsondoc=self.obographs,
(https://github.com/mapping-commons/sssom-py/runs/7661877722?check_suite_focus=true#step:8:48)
            prefix_map=self.metadata.prefix_map,
(https://github.com/mapping-commons/sssom-py/runs/7661877722?check_suite_focus=true#step:8:49)
            meta=self.metadata.metadata,
(https://github.com/mapping-commons/sssom-py/runs/7661877722?check_suite_focus=true#step:8:50)
        )
(https://github.com/mapping-commons/sssom-py/runs/7661877722?check_suite_focus=true#step:8:51)
        path = os.path.join(test_out_dir, "test_parse_obographs.tsv")
(https://github.com/mapping-commons/sssom-py/runs/7661877722?check_suite_focus=true#step:8:52)
        with open(path, "w") as file:
(https://github.com/mapping-commons/sssom-py/runs/7661877722?check_suite_focus=true#step:8:53)
            write_table(msdf, file)
(https://github.com/mapping-commons/sssom-py/runs/7661877722?check_suite_focus=true#step:8:54)
        self.assertEqual(
(https://github.com/mapping-commons/sssom-py/runs/7661877722?check_suite_focus=true#step:8:55)
            len(msdf.df),
(https://github.com/mapping-commons/sssom-py/runs/7661877722?check_suite_focus=true#step:8:56)
            9878,
(https://github.com/mapping-commons/sssom-py/runs/7661877722?check_suite_focus=true#step:8:57)
>           f"{self.obographs_file} has the wrong number of mappings.",
(https://github.com/mapping-commons/sssom-py/runs/7661877722?check_suite_focus=true#step:8:58)
        )
(https://github.com/mapping-commons/sssom-py/runs/7661877722?check_suite_focus=true#step:8:59)
E       AssertionError: 9876 != 9878 : /home/runner/work/sssom-py/sssom-py/tests/data/pato.json has the wrong number of mappings.

I do not know why this would change though? Thoughts?

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

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

This is super cool, make sure this works when installing from pip (remember these problems with loading from a package-local path in the past?).

The changing numbers are sort of expected, if the prefix map does not know a prefix, its not added to the table when parsing with obo graphs..

sssom/context.py Outdated
@@ -26,6 +29,11 @@ def get_jsonld_context():

:return: JSON-LD context
"""
schema_path = pkg_resources.resource_filename(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Does this work when installing with pip? Do we have a precedence in the code for this (loading the resource this way I mean). I feel schema_path should be in constants.py somehow, but up to you.

Copy link
Contributor

Choose a reason for hiding this comment

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

I forgot that schema_path was already implemented in constants.py. Updating code accordingly. The variable name is SCHEMA_YAML and is used in many parts of the project. So it definitely works since previous releases did not have any issues when we implemented sssom-schema.

@hrshdhgd
Copy link
Contributor

hrshdhgd commented Aug 4, 2022

@matentzn , ready for another review.

Copy link
Collaborator Author

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

I cant approve it with Github since I opened the PR but this is amazing, like - finally all your hard work engineering this stuff properly is paying off and changes are getting so much smaller and better. 3 months ago the same PR would have taken 1 week to implement with 100 lines of code.. Great!

@matentzn
Copy link
Collaborator Author

matentzn commented Aug 4, 2022

(approved)

@hrshdhgd hrshdhgd merged commit ad4d7c6 into master Aug 4, 2022
@hrshdhgd hrshdhgd deleted the fix-prefix-map-outdated branch August 4, 2022 15:04
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.

2 participants