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

Implementation to calculate conditional independencies #29

Merged
merged 100 commits into from Mar 10, 2021

Conversation

cthoyt
Copy link
Member

@cthoyt cthoyt commented Jan 22, 2021

Closes #24

This pull request implements an algorithm to calculate all conditional independencies in a given ADMG.

@cthoyt cthoyt mentioned this pull request Jan 22, 2021
9 tasks
@cthoyt
Copy link
Member Author

cthoyt commented Mar 9, 2021

@JosephCottam I think it's quite mature - can you please take care of all of the linting warnings then I will give a proper review?

@JosephCottam
Copy link
Contributor

@cthoyt I have addressed most of the formatting issues.

@cthoyt @djinnome I move that we ignore error N400 (backslash for line continuation). The PEP8 description centers on expressions with operator chaining. However, it is used for function chaining here. Though the warning can be cleared by adding parenthesis around the whole chain (see below), I think the line continuation version is more clear in what is going on.

    evidence = (pd.DataFrame(rows, columns=["left", "right", "given", "chi^2", "p", "dof"])
        .sort_values("p")
        .assign(**{"Holm–Bonferroni level": significance_level / pd.Series(range(len(rows) + 1, 0, -1))})
        .pipe(lambda df: df.assign(flagged=(df["p"] < df["Holm–Bonferroni level"])))
        .sort_values(["flagged", "dof"], ascending=False))```


@JosephCottam JosephCottam marked this pull request as ready for review March 9, 2021 20:11
@cthoyt
Copy link
Member Author

cthoyt commented Mar 9, 2021

@JosephCottam it looks like some of the tests started failing, but not related to code I changed. Are these non-deterministic, or is there something I missed?

@JosephCottam
Copy link
Contributor

@cthoyt To the best of my knowledge, all of the tests are deterministic.

@JosephCottam
Copy link
Contributor

@cthoyt Fixed the test case that started failing. The changes made to loading the asia example data broke that test case. It was just putting the file path into the dataframe, rather than loading the data from the file. Now it loads the data again!

@cthoyt
Copy link
Member Author

cthoyt commented Mar 10, 2021

@cthoyt Fixed the test case that started failing. The changes made to loading the asia example data broke that test case. It was just putting the file path into the dataframe, rather than loading the data from the file. Now it loads the data again!

Nice catch, sorry for the trouble

@cthoyt cthoyt merged commit 2cc1010 into main Mar 10, 2021
@cthoyt cthoyt deleted the calculate-conditional-independencies branch March 10, 2021 13:40
@cthoyt
Copy link
Member Author

cthoyt commented Mar 10, 2021

Thank you so much @JosephCottam for all of the effort! Looking forward to what's next

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.

Calculate all conditional independencies of an ADMG
2 participants