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

Normalize #75

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Normalize #75

wants to merge 7 commits into from

Conversation

cbizon
Copy link
Collaborator

@cbizon cbizon commented Sep 6, 2022

Define the normalize nodes operation

@@ -0,0 +1,170 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

Should this file be names something like 01_prenormalized_message.json instead of premerged?

@@ -0,0 +1,245 @@
{ "workflow": null,
Copy link
Member

Choose a reason for hiding this comment

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

Same with the name of this file

description: This operation updates the identifiers on qgraph and kgraph nodes to their preferred identifiers, and adds equivalent identifiers in a property for knodes. When two kgraph nodes normalize to the same preferred identifier, the two knodes are merged. The new node contain the union of the properties of the two original nodes. All edges attached to either of the two original nodes are now subsequently attached to the new merged knode. Qnodes are not merged, so that the structure of the query can be preserved. The updates to kgraph node identifiers also necessitates the updating of result node bindings.
examples:
- input: normalize_nodes/messages/01_premerged_message.json
output: normalize_nodes/messages/02_postmerged_message.json
Copy link
Member

Choose a reason for hiding this comment

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

Should there be a hyphen here?

@@ -0,0 +1,15 @@
id: normalize_nodes
Copy link
Member

Choose a reason for hiding this comment

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

Let's make this a non-unique operation (so there is only one way to do the normalization)

@cbizon
Copy link
Collaborator Author

cbizon commented Sep 8, 2022

Thanks @dkoslicki . I've updated the filenames / dash.

I'm not sure how to designate unique/nonunique. I took a look at sort and some others that I thought should be nonunique and didn't see any kind of annotation there. Can you point me in the right direction?

@dkoslicki dkoslicki removed the request for review from finnagin September 8, 2022 16:34
@dkoslicki
Copy link
Member

Here's an example of the unique flag: https://github.com/NCATSTranslator/OperationsAndWorkflows/blob/main/operations/fill.yml#L4
I assume we should set this to unique=False, but @uhbrar can check me on that

@uhbrar
Copy link
Collaborator

uhbrar commented Sep 8, 2022

@dkoslicki is right. This should be tagged as unique=False, although it actually doesn't need to be tagged at all, since the default is False. The current operations schema dictates that only unique operations need to be tagged as unique. Non-unique operations can be tagged as non-unique, or can be left un-tagged.

Here's an example: https://github.com/NCATSTranslator/OperationsAndWorkflows/blob/main/operations/bind.yml

@cbizon
Copy link
Collaborator Author

cbizon commented Sep 8, 2022

Aha, explains why I didn't see unique in the places I was looking. I went ahead and added it explicitly to this one.

@uhbrar
Copy link
Collaborator

uhbrar commented Sep 8, 2022

That shouldn't be a problem. However, before this is merged, the schema needs to be rebuilt first. This is done through schema/build/generate_schema.py

@cbizon
Copy link
Collaborator Author

cbizon commented Sep 8, 2022

Oh right. I've also updated the README with a reminder on the process.

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.

3 participants