Skip to content

[ENH] Implement minimal m-separator functionality for enumerating minimal m-separators and also checking if separating set is minimal #53

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

Merged
merged 18 commits into from
Feb 20, 2023

Conversation

jaron-lee
Copy link
Collaborator

Fixes #34

Changes proposed in this pull request:

  • Implements a function to detect minimal m-separators (if any) in a graph

Before submitting

  • I've read and followed all steps in the Making a pull request
    section of the CONTRIBUTING docs.
  • I've updated or added any relevant docstrings following the syntax described in the
    Writing docstrings section of the CONTRIBUTING docs.
  • If this PR fixes a bug, I've added a test that will fail without my fix.
  • If this PR adds a new feature, I've added tests that sufficiently cover my new functionality.

After submitting

  • All GitHub Actions jobs for my pull request have passed.

Signed-off-by: Jaron Lee <[email protected]>
Signed-off-by: Jaron Lee <[email protected]>
Signed-off-by: Jaron Lee <[email protected]>
@jaron-lee
Copy link
Collaborator Author

@adam2392 when you get a chance - I tried implementing minimal m-separators according to the Van der Zander paper. They have an example in figure 5 which I have written tests for. I have a failing test for minimal_m_separators(G, "X", "Y", i={"Z_1"}) in that graph.
I think I have implemented the algorithm (FINDMINSEP) correctly as it is written down in the paper, but there seems to be some issues with it generally? For example, when computing Z'' it's possible that the algorithm would search for a path from X to something in i, but in the third line of FINDMINSEP all nodes in i are removed, so there aren't any paths to i at all. I have tried both including and excluding i when this happens but couldn't get things to work. Currently I am excluding i, but in the failing test case this means that z is empty and so we fail to find a minimal m-separator, even though Z_1 is a valid minimal m-separator.

@jaron-lee
Copy link
Collaborator Author

Oh I think I figured it out, there is a missing line that should read:

"Remove from G' all nodes of I"

I should submit a PR on that paper...

Signed-off-by: Jaron Lee <[email protected]>
@adam2392
Copy link
Collaborator

Oh I think I figured it out, there is a missing line that should read:

"Remove from G' all nodes of I"

I should submit a PR on that paper...

If it ends up working, we should document the "change" relative to what's in the paper, so at least our docstring is a pseudo-source of truth :p

Signed-off-by: Jaron Lee <[email protected]>
Signed-off-by: Jaron Lee <[email protected]>
Signed-off-by: Jaron Lee <[email protected]>
Copy link
Collaborator

@adam2392 adam2392 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll do a more thorough check of the algo once we fix the nitpicks. Looking good!

@jaron-lee
Copy link
Collaborator Author

@adam2392 I think i basically fixed all the errors, but there is this one CI test failing. Doesn't look related to the code I've written. I vaguely recall we had this on another PR but I don't remember the fix.

@adam2392 adam2392 changed the title [WIP, ENH] Implement minimal m-separator functionality [ENH] Implement minimal m-separator functionality for enumerating minimal m-separators and also checking if separating set is minimal Feb 20, 2023
@adam2392
Copy link
Collaborator

adam2392 commented Feb 20, 2023

@adam2392 I think i basically fixed all the errors, but there is this one CI test failing. Doesn't look related to the code I've written. I vaguely recall we had this on another PR but I don't remember the fix.

Pushing a fix. The function just needs to be added to some autodoc (e.g. api.rst), so that way a rst page is generated for it, so then it can be auto-referenced by any corresponding

:func:`<func_name>`

@adam2392 adam2392 marked this pull request as ready for review February 20, 2023 16:56
Copy link
Collaborator

@adam2392 adam2392 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM minus some doc fixes.

@adam2392
Copy link
Collaborator

Will merge this once CIs give green light

@adam2392 adam2392 added this to the Release v0.1 milestone Feb 20, 2023
@adam2392 adam2392 merged commit 4e9cef8 into py-why:main Feb 20, 2023
@adam2392
Copy link
Collaborator

Thanks @jaron-lee !

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.

A function for enumerating d-separators in causal graphs
2 participants