py-why / pywhy-graphs

[Experimental] Causal graphs that are networkx-compliant for the py-why ecosystem.
https://py-why.github.io/pywhy-graphs/dev/index.html
MIT License
46 stars 8 forks source link

[ENH] Add ability to determine whether an inducing path exists between two nodes #78

Closed aryan26roy closed 1 year ago

aryan26roy commented 1 year ago

Closes #70

Changes proposed in this pull request:

Before submitting

After submitting

aryan26roy commented 1 year ago

The tetrad implementation for reference.

aryan26roy commented 1 year ago

@adam2392 one more question:

adam2392 commented 1 year ago

@adam2392 one more question:

  • Neither the children method nor the parents method finds nodes connected through a bi-directed edge. Is there a function that finds child and parent nodes connected through bi-directed edges as well?

You can query the networkx subgraph. Bidirected edges are stored using a nx.Graph.

codecov-commenter commented 1 year ago

Codecov Report

Merging #78 (b23a6bd) into main (2d6f1d3) will increase coverage by 0.09%. The diff coverage is 84.61%.

@@            Coverage Diff             @@
##             main      #78      +/-   ##
==========================================
+ Coverage   80.02%   80.11%   +0.09%     
==========================================
  Files          39       39              
  Lines        2838     2902      +64     
  Branches      759      775      +16     
==========================================
+ Hits         2271     2325      +54     
- Misses        408      412       +4     
- Partials      159      165       +6     
Impacted Files Coverage Δ
pywhy_graphs/algorithms/generic.py 85.11% <84.61%> (-0.46%) :arrow_down:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

aryan26roy commented 1 year ago

@adam2392 so it turns out the "parents" method doesn't work on the bidirected sub-graph. I am using the neighbors method.(since in a bidirected graph a neighbor is also a parent.)

adam2392 commented 1 year ago

can you add test cases as well?

You can just construct various graphs on paper that follow the definition or not to test.

aryan26roy commented 1 year ago

@adam2392 is this an appropriate amount of tests?

aryan26roy commented 1 year ago

@adam2392 The implementation seems to be correct and all the tests are passing as well. Want to review?

aryan26roy commented 1 year ago

@adam2392 I think I added all the tests you asked for. They worked without changing anything in the code. Can you take a look?

adam2392 commented 1 year ago

@aryan26roy thanks for iterating on this! Feel free to come to the Discord OH, or other time periods and ping the channel so we know we should discuss this if you have any questions.

Inducing paths are somewhat abstract at first tbh, so once the definition is clear, I think this will come out cleanly.

aryan26roy commented 1 year ago

@adam2392 I have incorporated all the suggestions you gave in previous reviews except the simplifying the checks one(not sure how I would simplify this further, it looks like a neat abstraction to me). Can you take a look?

aryan26roy commented 1 year ago

@adam2392 I made the suggested changes and some more.

aryan26roy commented 1 year ago

Yeah, patching the bugs has wreaked havoc on my naming system. Will make everything better in my next pass.

aryan26roy commented 1 year ago

@adam2392 The docs are not being built correctly, what am I doing wrong?

aryan26roy commented 1 year ago

@adam2392 do you want me to fix the networkx CI issue in another PR? I can bump up the CI from [3.8, 3.10] to [3.9,3.10].

aryan26roy commented 1 year ago

@adam2392 I have removed the example file. Do you want to merge now?

adam2392 commented 1 year ago

Thanks @aryan26roy !