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

Add initial working multigraph edge validator plus tests #107

Merged
merged 12 commits into from
Sep 30, 2021

Conversation

j6k4m8
Copy link
Member

@j6k4m8 j6k4m8 commented Sep 30, 2021

Based upon the work in #106 (thanks @celiibrendan!) and fixes #101.

@codecov-commenter
Copy link

codecov-commenter commented Sep 30, 2021

Codecov Report

Merging #107 (e827af5) into master (c60ba4e) will increase coverage by 0.41%.
The diff coverage is 97.95%.

❗ Current head e827af5 differs from pull request most recent head 1144b00. Consider uploading reports for the commit 1144b00 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #107      +/-   ##
==========================================
+ Coverage   88.03%   88.44%   +0.41%     
==========================================
  Files          24       25       +1     
  Lines        1763     1843      +80     
==========================================
+ Hits         1552     1630      +78     
- Misses        211      213       +2     
Impacted Files Coverage Δ
dotmotif/utils.py 57.14% <ø> (ø)
dotmotif/executors/test_neuprintexecutor.py 33.33% <33.33%> (ø)
dotmotif/tests/test_dm_flags.py 84.61% <87.50%> (ø)
dotmotif/executors/NetworkXExecutor.py 93.02% <95.91%> (+1.35%) ⬆️
dotmotif/__init__.py 95.34% <100.00%> (-0.06%) ⬇️
dotmotif/executors/GrandIsoExecutor.py 80.00% <100.00%> (+0.58%) ⬆️
dotmotif/executors/Neo4jExecutor.py 69.33% <100.00%> (ø)
dotmotif/executors/NeuPrintExecutor.py 41.93% <100.00%> (ø)
dotmotif/executors/test_dm_cypher.py 100.00% <100.00%> (ø)
dotmotif/executors/test_grandisoexecutor.py 100.00% <100.00%> (ø)
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c60ba4e...1144b00. Read the comment docs.

@j6k4m8
Copy link
Member Author

j6k4m8 commented Sep 30, 2021

@hpawa and @celiibrendan — I am finding (not unexpectedly) that proper multigraph support is going to take a little more engineering here. For now, I think it's reasonable to use the add-multigraph-support branch (there is GrandIsoExecutor support now, which means that it is quite fast for the types of use cases we've discussed before) while I work on the remaining corner cases.

Here's one such case, to illustrate what I mean:

A -> B [size > 10, size < 10]

Imagine there are two edges between host nodes a and b with size attributes 5 and 20. Right now, the logic doesn't fully support matching this as a success; that is, we only support matching ONE edge per attribute right now.

I'm going to work on building a better system when my brain isn't fried, but I think this is doable. Just... perhaps not tonight :)

@j6k4m8 j6k4m8 self-assigned this Sep 30, 2021
@j6k4m8 j6k4m8 merged commit 8e8ddb7 into master Sep 30, 2021
@j6k4m8 j6k4m8 deleted the add-multigraph-support branch September 30, 2021 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MultiDiGraph support
2 participants