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
47 stars 8 forks source link

[DOCS] Add an example for the Inducing Path function #83

Closed aryan26roy closed 1 year ago

aryan26roy commented 1 year ago

Closes #81

Changes proposed in this pull request:

Before submitting

After submitting

aryan26roy commented 1 year ago

@adam2392 How do you want me to proceed with this? Should I include scenarios where there is no path as well? And are the comments like this ok?

adam2392 commented 1 year ago

@adam2392 How do you want me to proceed with this? Should I include scenarios where there is no path as well? And are the comments like this ok?

The goal is to educate the user and serve as a quick reminder for yourself as a dev in case you need to review something.

I would proceed in sections:

  1. setup the data and graph
  2. draw the graph, perhaps comment that there is only 1 inducing path relative to L and S. This one inducing path is why the MAG in Figure 2 of the paper contains an extra adjacency
  3. Run the inducing path algorithm and actually verify this 1 inducing path and verify a few non-inducing path examples as well
  4. Show and explain what happens when we include the node into set S

Lmk if that makes sense

~Also I would add the example inside the intro/ folder, rather than making a new subfolder inside examples/ since this is a fundamental introduction to causal graphs.~

aryan26roy commented 1 year ago

@adam2392 What do you think?

adam2392 commented 1 year ago

Yes I think this is a good start and direction!

Can you take a look at the other examples and use "# %%" to break up into sections where it makes sense?

It makes the final rendering easier to read.

aryan26roy commented 1 year ago

@adam2392 How about now?

aryan26roy commented 1 year ago

@adam2392 can you take a look now?

adam2392 commented 1 year ago

Can you add a test in inducing_path that a graph should only have directed and/or bidirected edges? Otw, and inducing path doesn't make sense to run.

E.g. I would expect it to return an error if passed a PAG.

aryan26roy commented 1 year ago

Can you add a test in inducing_path that a graph should only have directed and/or bidirected edges? Otw, and inducing path doesn't make sense to run.

Do you want me to add it to this PR or create a new issue and a new PR?

adam2392 commented 1 year ago

You can just add it here since it's just a line just checking the types of edges present.

aryan26roy commented 1 year ago

@adam2392 there's one problem. There is no graph class that is only has directed and bidirected edges. Even ADMG can have undirected edges. Do you want me to just check if the argument is an instance of PAG or CPDAG instead?

adam2392 commented 1 year ago

@adam2392 there's one problem. There is no graph class that is only has directed and bidirected edges. Even ADMG can have undirected edges. Do you want me to just check if the argument is an instance of PAG or CPDAG instead?

I wouldn't check the class as we are going to eventually try to refactor that so there is no explicit PAG/CPDAG class.

Instead, I would check the edge types for right now 'directed', 'bidirected' keywords.

aryan26roy commented 1 year ago

Instead, I would check the edge types for right now 'directed', 'bidirected' keywords.

The problem is PAG would pass this test. It can have both of these edge types.

adam2392 commented 1 year ago

Instead, I would check the edge types for right now 'directed', 'bidirected' keywords.

The problem is PAG would pass this test. It can have both of these edge types.

We don't want any other edge. It can only have these types of edges.

aryan26roy commented 1 year ago

@adam2392 Unless you're ok with instances of PAG class being valid as long as they only have directed and bidirected edges. Otherwise there is no way to differentiate between the different types.

aryan26roy commented 1 year ago

@adam2392 I added the check. Also, I can't get the references to render, can you take a look at both?

aryan26roy commented 1 year ago

@adam2392 The last run didn't make any documentation artifacts?

adam2392 commented 1 year ago

Are you able to see the status of the circle CI docs build? The artifacts are listed there.

aryan26roy commented 1 year ago

Now I can see it! I guess github was glitching out for me. Anyway, are you happy with the current state?

adam2392 commented 1 year ago

Thanks @aryan26roy !