[ENH] CI tests for constraint-based structure learning #16

adam2392 · 2022-08-16T19:29:44Z

Signed-off-by: Adam Li adam2392@gmail.com

Follow up to: #14 to lead to #11

Changes proposed in this pull request:

Adds conditional independence testing to the submodule dodiscover.ci in a consistent API.
Also removed pylint from the style checking as it is too stringent.

Needs to update the networkx algorithms that will work on mixedEdgeGraphs.

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: Adam Li <adam2392@gmail.com>

pyproject.toml

dodiscover/ci/_script.py

dodiscover/ci/fisher_z_test.py

Signed-off-by: Adam Li <adam2392@gmail.com>

robertness · 2022-08-17T17:02:05Z

Looking at this now.

robertness · 2022-08-17T17:02:52Z

Makefile

+	@echo "Building documentation"
+	make -C doc/ clean
+	make -C doc/ html-noplot
+	cd doc/ && make view


Newline.
Does the makefile need to be commited?

No. This is just for my dev sake rn. I'm still wrangling upstream dependencies. I.e. pywhy-graphs. I'll remove it.

Actually jk I think it would be nice to have this. It makes the docs building a bit easier. If someone knows how to add a multi-step procedure to the poetry command build_docs, then maybe not tho...

If it's something scripted like this I would generally model this under a bash file and invoke the bash file via a poetry script.

adam2392 · 2022-08-17T17:25:10Z

I'll get these CI green and then work on the PC part.

This turned out to be a lot of work since we're trying to keep integration with networkx proposal, and pywhy-graphs. But I think it'll be worth it once we see the PC algo implemented with these in mind :).

robertness · 2022-08-17T20:19:56Z

dodiscover/_protocol.py

+import networkx as nx
+
+
+class GraphProtocol(Protocol):


Interesting. What is the rationale for using duck typing instead of subclassing? Is it normal to use protocols with methods that all need to be overridden?

Protocols just are Python's way of defining an abstract interface. So in the code, note that pywhy-graphs is never imported. And does not need to be. This is in line w/ discussions we had awhile back where the Dev team requested that we don't explicitly have to rely on pywhy's implementation of causal graphs.

This GraphProtocol class then allows a user to define their own graph class, as long as it adheres to the API, then it "should" still work. In addition, this provides mypy type checking w/o having to import pywhy-graphs explicitly.

Just to add an explicit example:

if someone from say ananke wants to make their graph implementation work with the implementation of the algorithms here that rely on a GraphProtocol, then they just need to modify their API to have the functions specified.

I personally am okay w/ being opinionated and requiring users to use pywhy-graphs directly and providing a transformation module for converting graphs from common formats, but I think we can always swap that easily using ctrl+f+replace

dodiscover/ci/_script.py

robertness · 2022-08-17T20:45:11Z

dodiscover/ci/base.py

+
+    @abstractmethod
+    def test(
+        self, df: pd.DataFrame, x_var: Any, y_var: Any, z_covariates: Any = None


Why are x_var, y_var and z_covariates of the Any type? Categoricals, integers, and floats right?

They can be anything that a pandas dataframe column supports. I wasn't 100% sure if Categoricals, integers, and floats are sufficient. Are they?

robertness · 2022-08-17T21:10:37Z

dodiscover/ci/fisher_z_test.py

+
+    def test(
+        self, df: pd.DataFrame, x_var: Any, y_var: Any, z_covariates: Any = None
+    ) -> Tuple[float, float]:


Might be useful to create types for the test stat and the p-value as this will be used in multiple places.

Aren't they just floats tho? With pvalue being constrained between 0 and 1

Signed-off-by: Adam Li <adam2392@gmail.com>

dodiscover/ci/g_test.py

dodiscover/ci/oracle.py

tests/unit_tests/ci/test_fisherz_test.py

robertness

LGTM. My only issues are with some of the type hints.

@adam2392 can you comment here on why some flavors are left out? For example:

mutual information and asymptotic chi-square test for discrete variables, perhaps with a James-Stein estimator
Monte Carlo permutation testing and sequential Monte Carlo permutation testing
Jonckheere-Terpstra test for ordinal variables

adam2392 · 2022-08-18T02:09:56Z

Will address the code comments tonight.

Re the additional CI tests: just never implemented them myself :p. It would be nice to get a community contribution for those. Or one of us can tackle it when there's time?

Signed-off-by: Adam Li <adam2392@gmail.com>

codecov-commenter · 2022-08-18T03:55:23Z

Codecov Report

Merging #16 (bbb128b) into main (7533e90) will increase coverage by 16.90%.
The diff coverage is 77.15%.

@@             Coverage Diff             @@
##             main      #16       +/-   ##
===========================================
+ Coverage   60.00%   76.90%   +16.90%     
===========================================
  Files           1        7        +6     
  Lines           5      342      +337     
  Branches        0       55       +55     
===========================================
+ Hits            3      263      +260     
- Misses          2       63       +61     
- Partials        0       16       +16

Impacted Files	Coverage Δ
dodiscover/ci/oracle.py	`47.36% <47.36%> (ø)`
dodiscover/_protocol.py	`60.86% <60.86%> (ø)`
dodiscover/ci/g_test.py	`61.01% <61.01%> (ø)`
dodiscover/ci/base.py	`91.66% <91.66%> (ø)`
dodiscover/ci/kernel_test.py	`92.64% <92.64%> (ø)`
dodiscover/ci/fisher_z_test.py	`96.55% <96.55%> (ø)`

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

adam2392 · 2022-08-18T13:52:08Z

Gonna merge this for now to move forward w/ the next step. Left some comments open w/ my responses tho.

adam2392 added 2 commits August 16, 2022 15:28

WIP until graphs converge

85585eb

Signed-off-by: Adam Li <adam2392@gmail.com>

WIP until graphs converge

c61b0d7

Signed-off-by: Adam Li <adam2392@gmail.com>

adam2392 requested a review from darthtrevino August 16, 2022 19:31

darthtrevino approved these changes Aug 16, 2022

View reviewed changes

pyproject.toml Show resolved Hide resolved

dodiscover/ci/_script.py Outdated Show resolved Hide resolved

dodiscover/ci/fisher_z_test.py Outdated Show resolved Hide resolved

adam2392 added 2 commits August 16, 2022 17:47

Fix CI

38d9a72

Signed-off-by: Adam Li <adam2392@gmail.com>

Fix CI

8d1f841

Signed-off-by: Adam Li <adam2392@gmail.com>

robertness reviewed Aug 17, 2022

View reviewed changes

dodiscover/ci/_script.py Outdated Show resolved Hide resolved

robertness reviewed Aug 17, 2022

View reviewed changes

adam2392 added 2 commits August 17, 2022 17:13

Fix ci

f8d8dcc

Signed-off-by: Adam Li <adam2392@gmail.com>

Fix ci

4835a6e

Signed-off-by: Adam Li <adam2392@gmail.com>

robertness reviewed Aug 18, 2022

View reviewed changes

dodiscover/ci/g_test.py Outdated Show resolved Hide resolved

robertness reviewed Aug 18, 2022

View reviewed changes

dodiscover/ci/oracle.py Show resolved Hide resolved

robertness reviewed Aug 18, 2022

View reviewed changes

tests/unit_tests/ci/test_fisherz_test.py Show resolved Hide resolved

robertness approved these changes Aug 18, 2022

View reviewed changes

adam2392 added 4 commits August 17, 2022 22:34

Adding refs

b39b4d9

Signed-off-by: Adam Li <adam2392@gmail.com>

Adding refs

f599e3d

Signed-off-by: Adam Li <adam2392@gmail.com>

Fix ci

1f80cb4

Signed-off-by: Adam Li <adam2392@gmail.com>

Fix docs

bbb128b

Signed-off-by: Adam Li <adam2392@gmail.com>

adam2392 merged commit a67b95c into py-why:main Aug 18, 2022

adam2392 deleted the ci branch August 18, 2022 13:52

adam2392 mentioned this pull request Aug 18, 2022

Add additional CI tests: Mutual Information, Chi square, Monte Carlo #17

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ENH] CI tests for constraint-based structure learning #16

[ENH] CI tests for constraint-based structure learning #16

adam2392 commented Aug 16, 2022 •

edited

Loading

robertness commented Aug 17, 2022

robertness Aug 17, 2022

adam2392 Aug 17, 2022

adam2392 Aug 17, 2022

darthtrevino Aug 17, 2022

adam2392 commented Aug 17, 2022

robertness Aug 17, 2022 •

edited

Loading

adam2392 Aug 17, 2022

adam2392 Aug 17, 2022

robertness Aug 17, 2022

adam2392 Aug 17, 2022

robertness Aug 17, 2022

adam2392 Aug 17, 2022

robertness left a comment

adam2392 commented Aug 18, 2022

codecov-commenter commented Aug 18, 2022

adam2392 commented Aug 18, 2022

[ENH] CI tests for constraint-based structure learning #16

[ENH] CI tests for constraint-based structure learning #16

Conversation

adam2392 commented Aug 16, 2022 • edited Loading

Before submitting

After submitting

robertness commented Aug 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adam2392 commented Aug 17, 2022

robertness Aug 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertness left a comment

Choose a reason for hiding this comment

adam2392 commented Aug 18, 2022

codecov-commenter commented Aug 18, 2022

Codecov Report

adam2392 commented Aug 18, 2022

adam2392 commented Aug 16, 2022 •

edited

Loading

robertness Aug 17, 2022 •

edited

Loading