[ENH] Add PC algorithm #18

adam2392 · 2022-08-18T14:55:00Z

Fixes #11
Closes: #10

Changes proposed in this pull request:

PC algorithm implementation with separate skeleton discovery stage (has all the features of causal-learn I think(?), except for missing-value support)
Adds the abstract class for Constraint Based learning algorithms
Adds the Context class
Adds an example showing how to run PC algorithm
Adds a metric submodule for comparing graphs

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>

adam2392 · 2022-08-18T16:43:54Z

Here are the docs output:

https://output.circle-artifacts.com/output/job/42854568-1d11-48d3-b89a-660449f4b9c0/artifacts/0/dev/index.html

Although not sure how to manage the versions and what they look like. It seemed that 0.0.0 was needed to be hard coded right? Do we just manually increment that when we make release?
@darthtrevino

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

codecov-commenter · 2022-08-18T18:06:45Z

Codecov Report

Merging #18 (28c1fa6) into main (a67b95c) will decrease coverage by 0.66%.
The diff coverage is 73.45%.

@@            Coverage Diff             @@
##             main      #18      +/-   ##
==========================================
- Coverage   76.90%   76.23%   -0.67%     
==========================================
  Files           7       13       +6     
  Lines         342      787     +445     
  Branches       55      159     +104     
==========================================
+ Hits          263      600     +337     
- Misses         63      138      +75     
- Partials       16       49      +33

Impacted Files	Coverage Δ
dodiscover/constraint/pcalg.py	`65.41% <65.41%> (ø)`
dodiscover/constraint/utils.py	`68.75% <68.75%> (ø)`
dodiscover/context.py	`69.56% <69.56%> (ø)`
dodiscover/_protocol.py	`61.53% <73.91%> (+0.66%)`	⬆️
dodiscover/constraint/skeleton.py	`75.88% <75.88%> (ø)`
dodiscover/constraint/_classes.py	`81.15% <81.15%> (ø)`
dodiscover/metrics.py	`91.66% <91.66%> (ø)`
dodiscover/ci/g_test.py	`62.71% <0.00%> (+1.69%)`	⬆️
dodiscover/ci/oracle.py	`100.00% <0.00%> (+52.63%)`	⬆️

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

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

adam2392 · 2022-08-18T19:53:10Z

https://output.circle-artifacts.com/output/job/4ffd2aba-0ca5-4a98-ab4d-29c694a00382/artifacts/0/dev/index.html

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

bloebp · 2022-08-18T22:38:35Z

This PR is quite big. Would it be possible to have each of the listed changes as a separate PR? Otherwise it is almost impossible that there is a proper review. Optimally, a PR should not have more than ~250 lines (preferably less). Except this is simply moving stuff over that has been review before already.

adam2392 · 2022-08-18T22:47:08Z

That's doable! The challenge here is that the CI is still evolving as well. Sorry some files are changing there.

If you're okay with the skeleton, abstract class, etc. being PRed without tests first that's fine.

Then the final PR for the PC algo would have the corresponding tests?

bloebp · 2022-08-18T23:11:48Z

You can also choose another PR (i.e. the branch) as a reference ('based on'), then if you make changes in, e.g., the CI stuff, it would be reflected here as well. Also, if there are inconsistencies, you can simply resolve them once the PR is approved. E.g., if the PR is about the PC-algorithm, we don't care about the details of the CI tests as long as we assume that there is 'something'. Then once the PR for the PC-algorithm is approved, you can simply adjust/rebase it to the latest changes of the CI tests.

Wouldn't it be possible to add tests for the classes independently of the PC algo? Just some basic tests (if they are necessary at all), like adding/deleting edges behave as expected etc. And then you can add further ones with the PC commit.

robertness · 2022-08-19T03:59:04Z

doc/api.rst

+   EquivalenceClassProtocol
+
+Constraint-Based Structure Learning


Shouldn't these have gone in the previous PR? Perhaps they can go into a separate PR?

robertness · 2022-08-19T04:02:01Z

doc/conf.py

-}
+# Add pygraphviz png scraper, if available
+try:
+    from pygraphviz.scraper import PNGScraper


Could graphviz visualiation be its own PR?

robertness · 2022-08-19T04:04:14Z

doc/whats_new/v0.1.rst

@@ -28,6 +28,7 @@ Changelog

 - |Feature| Implement continuous integration and repository docs, by `Adam Li`_ (:pr:`15`)
 - |Feature| Implement conditional independence tests under the ``dodiscover.ci`` submodule, by `Adam Li`_ (:pr:`16`)
+- |Feature| Implement PC algorithm and skeleton for constraint-based structure learning algorithms under the ``dodiscover.constraint`` submodule, by `Adam Li`_ (:pr:`18`)


@adam2392 are you updating this change log manually? Isn't this something we could handle with semantic commits? @darthtrevino

Yeah rn it's just a per PR changelog. What's semantic commits? :0

https://python-semantic-release.readthedocs.io/ - this library will parse git commit messages for changelog information

https://pypi.org/project/semversioner/ - this project will let you batch up change documents that are applied into the changelog via their CLI.

I kind of prefer the latter approach; commit-message parsing can be flaky if contributors aren't aware of the requirement. We use similar tooling in our JS projects, and we require that every PR has a semver-impact document associated with it.

commit-message parsing can be flaky

I've certainly been guilty of crummy semantic commit messages. I'm inclined to go with whatever you think is best @darthtrevino . I'd like to employ what CI tech that can help these PR's can get leaner. The leaner they are, the easier it is for me to onboard new contributors.

adam2392 · 2022-08-19T16:39:42Z

This PR is quite big. Would it be possible to have each of the listed changes as a separate PR? Otherwise it is almost impossible that there is a proper review. Optimally, a PR should not have more than ~250 lines (preferably less). Except this is simply moving stuff over that has been review before already.

@bloebp @robertness and @darthtrevino I've now broken up the PR a bit. I'm leaving this open to demonstrate the PC example for discussion, but for the code itself:

first it would be good to merge [DOCS] Fix docs and certain ci builds to include pygraphviz #19 to consolidate issues with the CI
second, I have added the skeleton learning method, which can be its own PR in [ENH] Skeleton learning method #20
Once those are merged in, I can then add the abstract base class for constraint-based structure learning algos. [ENH] Abstract base class for constraint based structure learning #21
Finally, the PC algo along with its tests and examples can be added in this PR after it is rebased on top of the main branch

adam2392 · 2022-08-19T16:41:24Z

^ Apologies for the gigantic PRs. I am more used to doing things in bite-sized chunks, but getting a repo setup sometimes takes a bit of wrangling. Will be better about it :p

Hopefully the breakups now make reviewing more streamlined and easy.

darthtrevino

LGTM; just a question about pygraphviz install

darthtrevino · 2022-08-19T17:05:50Z

.circleci/config.yml

+      - run:
+          name: Check poetry package versions
+          command: |
+            poetry run pip install --upgrade --progress-bar off pygraphviz


This seems weird - why not add pygraphviz as a listed dependency in pyproject.toml? In general, my approach in dowhy has been to avoid pip entirely and model all dependencies inside of `pyproject.toml'. When we have these tacit installs it makes setting up a dev environment kind of tricky.

Good point. I added it to the pyproject.toml and updated the poetry.lock file in #19

darthtrevino · 2022-08-19T17:10:19Z

doc/whats_new/v0.1.rst

@@ -28,6 +28,7 @@ Changelog

 - |Feature| Implement continuous integration and repository docs, by `Adam Li`_ (:pr:`15`)
 - |Feature| Implement conditional independence tests under the ``dodiscover.ci`` submodule, by `Adam Li`_ (:pr:`16`)
+- |Feature| Implement PC algorithm and skeleton for constraint-based structure learning algorithms under the ``dodiscover.constraint`` submodule, by `Adam Li`_ (:pr:`18`)


https://python-semantic-release.readthedocs.io/ - this library will parse git commit messages for changelog information

https://pypi.org/project/semversioner/ - this project will let you batch up change documents that are applied into the changelog via their CLI.

I kind of prefer the latter approach; commit-message parsing can be flaky if contributors aren't aware of the requirement. We use similar tooling in our JS projects, and we require that every PR has a semver-impact document associated with it.

adam2392 · 2022-08-30T22:00:59Z

Closed in favor of #30

Initial commit

515ee69

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

adam2392 force-pushed the pc2 branch from 8a9c153 to 179144f Compare August 18, 2022 15:15

adam2392 added 5 commits August 18, 2022 14:05

Adding example

a20b919

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

Fixing CI

43fb5e1

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

Fixed readme and typing

ed7bdc2

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

Fix circleci

2698a2b

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

Fix gh build

cfa9cf0

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

adam2392 force-pushed the pc2 branch from 03bfa3f to cfa9cf0 Compare August 18, 2022 18:05

adam2392 added 6 commits August 18, 2022 14:27

Try docs

fddf083

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

Try again...

3b2ac14

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

Fix style

3423705

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

Fix style

978ad0a

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

Adding refs

c16f26b

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

Adding refs

c8f819a

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

add time

28c1fa6

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

adam2392 requested review from robertness and darthtrevino August 18, 2022 22:15

robertness reviewed Aug 19, 2022

View reviewed changes

This was referenced Aug 19, 2022

[DOCS] Fix docs and certain ci builds to include pygraphviz #19

Merged

[ENH] Skeleton learning method #20

Merged

[ENH] Abstract base class for constraint based structure learning #21

Merged

darthtrevino approved these changes Aug 19, 2022

View reviewed changes

robertness approved these changes Aug 19, 2022

View reviewed changes

adam2392 closed this Aug 30, 2022

adam2392 deleted the pc2 branch August 30, 2022 22:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ENH] Add PC algorithm #18

[ENH] Add PC algorithm #18

adam2392 commented Aug 18, 2022 •

edited

Loading

adam2392 commented Aug 18, 2022 •

edited

Loading

codecov-commenter commented Aug 18, 2022 •

edited

Loading

adam2392 commented Aug 18, 2022

bloebp commented Aug 18, 2022

adam2392 commented Aug 18, 2022

bloebp commented Aug 18, 2022 •

edited

Loading

robertness Aug 19, 2022

robertness Aug 19, 2022

robertness Aug 19, 2022

adam2392 Aug 19, 2022

darthtrevino Aug 19, 2022

robertness Aug 19, 2022

adam2392 commented Aug 19, 2022

adam2392 commented Aug 19, 2022

darthtrevino left a comment

darthtrevino Aug 19, 2022

adam2392 Aug 19, 2022

darthtrevino Aug 19, 2022

adam2392 commented Aug 30, 2022

[ENH] Add PC algorithm #18

[ENH] Add PC algorithm #18

Conversation

adam2392 commented Aug 18, 2022 • edited Loading

Before submitting

After submitting

adam2392 commented Aug 18, 2022 • edited Loading

codecov-commenter commented Aug 18, 2022 • edited Loading

Codecov Report

adam2392 commented Aug 18, 2022

bloebp commented Aug 18, 2022

adam2392 commented Aug 18, 2022

bloebp commented Aug 18, 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

adam2392 commented Aug 19, 2022

adam2392 commented Aug 19, 2022

darthtrevino left a 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

adam2392 commented Aug 30, 2022

adam2392 commented Aug 18, 2022 •

edited

Loading

adam2392 commented Aug 18, 2022 •

edited

Loading

codecov-commenter commented Aug 18, 2022 •

edited

Loading

bloebp commented Aug 18, 2022 •

edited

Loading