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

first version of RDF conversion #337

Merged
merged 2 commits into from Apr 18, 2024
Merged

first version of RDF conversion #337

merged 2 commits into from Apr 18, 2024

Conversation

LoesvdBiggelaar
Copy link
Collaborator

@LoesvdBiggelaar LoesvdBiggelaar commented Apr 17, 2024 •
edited

I build upon the pull request of @supermaxiste.
the output will now

  • add properties to the output
  • convert biocypher 'edges' to rdf 'nodes'
  • add properties of the biocypher 'edges' to the rdf 'nodes'
  • take care of some uriRefs

what still should/could be improved:

  • uri processing
  • unit test

@slobentanzer
Copy link
Collaborator

Thanks a lot, @LoesvdBiggelaar! @nilskre could you do a first pass on this?

@LoesvdBiggelaar
Copy link
Collaborator Author

LoesvdBiggelaar commented Apr 17, 2024 •
edited

Thanks a lot, @LoesvdBiggelaar! @nilskre could you do a first pass on this?

Let me know if and what I should update the code based on the feedback!

@nilskre
Copy link
Collaborator

nilskre commented Apr 17, 2024

Thanks a lot for the PR @LoesvdBiggelaar
@slobentanzer Yes, I will have a look at it.

nilskre
nilskre reviewed Apr 17, 2024
View reviewed changes
Copy link
Collaborator

@nilskre nilskre left a comment

Choose a reason for hiding this comment

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

Great work so far! Thanks a lot.

One question: How do you debug the generated RDF files? Do you load them in e.g. Protege to visualize them?

I have added some code-specific comments in the review and am happy to hear your opinion about them.
Some general comments:

  • At the moment the tests are failing. Before merging they need to pass.
  • It would be great to have a docs page describing how to use the RDF output adapter (similar to the neo4j output docs.
  • Not sure, if you already have used the pre-commit hooks

If there are any questions don't hesitate to ask.

biocypher/write/graph/_rdf.py Outdated Show resolved Hide resolved
biocypher/write/graph/_rdf.py Outdated Show resolved Hide resolved
biocypher/write/graph/_rdf.py Outdated Show resolved Hide resolved
biocypher/write/graph/_rdf.py Outdated Show resolved Hide resolved
biocypher/write/graph/_rdf.py Outdated Show resolved Hide resolved
biocypher/write/graph/_rdf.py Outdated Show resolved Hide resolved
biocypher/write/graph/_rdf.py Outdated Show resolved Hide resolved
biocypher/write/graph/_rdf.py Outdated Show resolved Hide resolved
test/write/graph/test_rdf.py Outdated Show resolved Hide resolved
test/write/graph/test_rdf.py Outdated

# check if the number of nodes and relations are correct
assert len(set(g.subjects())) == 15 # generated 4 nodes per type (8 in total) and 4 'edges'. together with the class definition (+3) makes 15 in total
assert len(set(g.subject_objects())) == 47 # all the triples; between nodes, but also the converted properties of nodes and the 'edges'.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good initial test (also for debugging/reviewing quite helpful :)
Furthermore, I would expect, that the content of the rdf Graph is tested in detail (id, name, properties, ...).

@LoesvdBiggelaar
Copy link
Collaborator Author

Great work so far! Thanks a lot.

One question: How do you debug the generated RDF files? Do you load them in e.g. Protege to visualize them?

I have added some code-specific comments in the review and am happy to hear your opinion about them. Some general comments:

  • At the moment the tests are failing. Before merging they need to pass.
  • It would be great to have a docs page describing how to use the RDF output adapter (similar to the neo4j output docs.
  • Not sure, if you already have used the pre-commit hooks

If there are any questions don't hesitate to ask.

Thanks for the feedback! I'll process them

@LoesvdBiggelaar
Copy link
Collaborator Author

regarding the tests, only the test/test_integration.py is failing. However, I don't think I changed anything there. Could someone explain what that test is actually testing and how I influenced this test with the RDF code?

@slobentanzer
Copy link
Collaborator

regarding the tests, only the test/test_integration.py is failing. However, I don't think I changed anything there. Could someone explain what that test is actually testing and how I influenced this test with the RDF code?

Hi Loes,
you are right, this is a bug (that somehow did not trigger the test to fail before, which is mysterious). I wanted to fix it, but I cannot push to the PR (You don't have permissions to push to "thehyve/biocypher" on GitHub.). Can you allow maintainers to modify this PR?

@LoesvdBiggelaar
Copy link
Collaborator Author

LoesvdBiggelaar commented Apr 18, 2024 •
edited

you are right, this is a bug (that somehow did not trigger the test to fail before, which is mysterious). I wanted to fix it, but I cannot push to the PR (You don't have permissions to push to "thehyve/biocypher" on GitHub.). Can you allow maintainers to modify this PR?

I cannot find anywere a button allow edits by maintainers. I looked at this pull request, the branch in thehyve/biocypher main and rdf. I also tried to add you personally to the repository but that doesn't work either. maybe I'm missing something?

I noticed that this pull request only exists in biocypher/biocypher and not on thehyve/biocypher. maybe that has something to do with it?

@slobentanzer
Copy link
Collaborator

I can't see your version of the PR, but the button/checkbox should be there. https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/creating-a-pull-request-from-a-fork#

Not completely sure if you can activate it only when you create the PR though... it would definitely be very helpful if we could commit to the PR directly.

@nilskre
Copy link
Collaborator

nilskre commented Apr 18, 2024

We were probably investigating this at the same time @slobentanzer .
I have pushed the fix here for now.

@slobentanzer
Copy link
Collaborator

slobentanzer commented Apr 18, 2024 •
edited

Yes, we did. :)

Should we just merge this PR into the biocypher:rdf branch and create a new PR to main from there? @LoesvdBiggelaar I can add you to the BioCypher org, so you can also push to that branch directly.

@LoesvdBiggelaar
Copy link
Collaborator Author

Yes, we did. :)

Should we just merge this PR into the biocypher:rdf branch and create a new PR to main from there? @LoesvdBiggelaar I can add you to the BioCypher org, so you can also push to that branch directly.

I am working on a bit of documentation still. so could we merge this PR when that commit is also added?

@slobentanzer
Copy link
Collaborator

Sure, I'll do it when you give me the go. :)

@LoesvdBiggelaar
Copy link
Collaborator Author

Sure, I'll do it when you give me the go. :)

Done! we can merge

@slobentanzer slobentanzer changed the base branch from main to rdf April 18, 2024 14:29
@slobentanzer slobentanzer merged commit c58d64f into biocypher:rdf Apr 18, 2024
2 of 17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nilskre nilskre nilskre left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@LoesvdBiggelaar @slobentanzer @nilskre