-
Notifications
You must be signed in to change notification settings - Fork 27
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
Added the function construct_pdp_query to build cypher queries that c… #30
Conversation
…alculate the path degree product
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice. I haven't had time to look in depth.
Can we add some tests? I am not sure whether the package contains any test for this module, but it would be helpful to make sure that we're getting the expected output for each of the options.
I added some tests for construct_pdp_query both with and without the dwpc provided to ensure that they give the same results. I also went ahead and wrote tests for construct_dwpc_query because it is similar. I had to add the neo4j package to the test dependencies to fix the build error |
…ey would run faster and with less memory (and as a result stop breaking travis
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very impressive PR! I think these improvements will be really helpful to this project.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ben-heil you can make a follow up PR with the requested changed.
@zietzm I am going to leave the merge as is on the PR. I believe @zietzm intended to squash merge rather than make a merge commit. While we generally prefer squash merges, I think it's best not to rewrite master in this instance. @zietzm you'll notice their are various merge options in the github dropdown menu when merging.
dwpc = dwpc) | ||
# If the dwpc isn't provided, we'll have to calculate it before the PDP. | ||
# Doing so roughly doubles the query execution time, as it effectively | ||
# runs the query twice returning different degrees of aggregation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's open a stack overflow issue with the neo4j
tag on whether we can do a sum aggregation and then combine that with the pre-aggregated result table all in Cypher.
|
||
# Unique node constraint (pevent paths with duplicate nodes) | ||
if unique_nodes == 'nested': | ||
unique_nodes_query = '\nAND ALL (x IN nodes(path) WHERE size(filter(z IN nodes(path) WHERE z = x)) = 1)' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible we can put all the code duplicated by construct_dwpc_query
in local functions that both methods call?
|
||
dwpc = results['DWPC'] | ||
|
||
assert dwpc == pytest.approx(0.03287590886921623) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's also add tests that evaluate that the expected Cypher query is exported for a given metagraph, but don't actually execute the Cypher.
Addressed the comments and added the code to #32. I would have created a new pull request, but Github automatically updated the existing one when I pushed to my personal account |
Construct_pdp_query acts like construct_dwpc_query, except that it returns the path degree product instead. This function also takes in the dwpc so that the path degree products can be represented as a fraction of the total dwpc. If the dwpc is not provided, a subquery will be added to calculate it.