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

Added the function construct_pdp_query to build cypher queries that c… #30

Merged
merged 6 commits into from
Jan 10, 2019

Conversation

ben-heil
Copy link
Contributor

@ben-heil ben-heil commented Jan 8, 2019

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.

Copy link
Member

@dhimmel dhimmel left a 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.

@ben-heil
Copy link
Contributor Author

ben-heil commented Jan 9, 2019

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

Copy link
Member

@zietzm zietzm left a 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.

setup.py Show resolved Hide resolved
hetio/neo4j.py Outdated Show resolved Hide resolved
hetio/neo4j.py Show resolved Hide resolved
hetio/neo4j.py Show resolved Hide resolved
test/neo4j_test.py Show resolved Hide resolved
@zietzm zietzm merged commit 956f0de into hetio:master Jan 10, 2019
Copy link
Member

@dhimmel dhimmel left a 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.
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 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)'
Copy link
Member

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)
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 also add tests that evaluate that the expected Cypher query is exported for a given metagraph, but don't actually execute the Cypher.

@ben-heil
Copy link
Contributor Author

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

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