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

[ENH] Skeleton learning method #20

Merged
merged 23 commits into from
Aug 24, 2022
Merged

[ENH] Skeleton learning method #20

merged 23 commits into from
Aug 24, 2022

Conversation

adam2392
Copy link
Collaborator

Follow on to: #19
And part of #18

Changes proposed in this pull request:

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.

dodiscover/constraint/skeleton.py Outdated Show resolved Hide resolved
dodiscover/constraint/skeleton.py Outdated Show resolved Hide resolved
dodiscover/constraint/skeleton.py Outdated Show resolved Hide resolved
dodiscover/constraint/skeleton.py Outdated Show resolved Hide resolved
dodiscover/constraint/skeleton.py Outdated Show resolved Hide resolved
dodiscover/constraint/skeleton.py Outdated Show resolved Hide resolved
dodiscover/constraint/utils.py Outdated Show resolved Hide resolved
dodiscover/constraint/utils.py Outdated Show resolved Hide resolved
dodiscover/context.py Show resolved Hide resolved
tests/unit_tests/constraint/test_skeleton.py Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Aug 22, 2022

Codecov Report

Merging #20 (ead714f) into main (b56a106) will decrease coverage by 1.62%.
The diff coverage is 68.32%.

@@            Coverage Diff             @@
##             main      #20      +/-   ##
==========================================
- Coverage   76.83%   75.21%   -1.63%     
==========================================
  Files           8       11       +3     
  Lines         367      585     +218     
  Branches       59      116      +57     
==========================================
+ Hits          282      440     +158     
- Misses         66      104      +38     
- Partials       19       41      +22     
Impacted Files Coverage Δ
dodiscover/constraint/utils.py 46.15% <46.15%> (ø)
dodiscover/ci/oracle.py 86.95% <50.00%> (+30.43%) ⬆️
dodiscover/context.py 67.39% <67.39%> (ø)
dodiscover/constraint/skeleton.py 70.51% <70.51%> (ø)
dodiscover/_protocol.py 61.53% <75.00%> (+0.66%) ⬆️
dodiscover/ci/g_test.py 63.63% <0.00%> (+1.65%) ⬆️

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

@adam2392
Copy link
Collaborator Author

This now gives us a generic backbone for all the PC/FCI algo variants, where the first step is "learning a skeleton" from CI testing of the data.

The LearnSkeleton can most likely be subclassed for additional structure such as computing the "possibly d-separating sets" in RFCI/FCI and evaluation of F-nodes when given interventional data.

Comment on lines 92 to 93
# the tests get the correct independences when alpha is at 0.01
alpha = 0.01
Copy link
Member

@bloebp bloebp Aug 23, 2022

Choose a reason for hiding this comment

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

Hmm, 'tuning' the parameter in a way to make the test work is maybe not a good idea. There might be something wrong with the implementation, which is then not visible due to the tuned parameter. If this is purely due to the flakiness of the (statistical) tests, consider rerunning them them when they fail with newly generated data. If it fails, e.g., 3x in a row, then it is a good indication to double check the implementation (and this could be the independence test here). See for instance https://github.com/py-why/dowhy/blob/main/tests/gcm/independence_test/test_kernel.py#L12, where we use flaky for exactly these kind of flaky scenarios.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

IIRC I think I took the data from pcalg of the R package... so it's not actually generated on the fly.

I assumed it was fine since the tests passed other types of simulated data, but let me actually investigate.

I might just play around w/ simulating some new data instead from an SCM.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh wait I just checked again and 0.05 works. I think this might've been a different private project that I was working on. The alpha level works for 0.05 (default) and 0.01.

adam2392 and others added 19 commits August 23, 2022 20:25
Signed-off-by: Adam Li <adam2392@gmail.com>
Signed-off-by: Adam Li <adam2392@gmail.com>
Signed-off-by: Adam Li <adam2392@gmail.com>
Signed-off-by: Adam Li <adam2392@gmail.com>
Signed-off-by: Adam Li <adam2392@gmail.com>
* Fix docs and certain ci builds for pygraphviz

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

* Add pygraphviz

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

* Add pygraphviz

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

Signed-off-by: Adam Li <adam2392@gmail.com>
Signed-off-by: Adam Li <adam2392@gmail.com>
Signed-off-by: Adam Li <adam2392@gmail.com>
Signed-off-by: Adam Li <adam2392@gmail.com>
Signed-off-by: Adam Li <adam2392@gmail.com>
Signed-off-by: Adam Li <adam2392@gmail.com>
Signed-off-by: Adam Li <adam2392@gmail.com>
Signed-off-by: Adam Li <adam2392@gmail.com>
Signed-off-by: Adam Li <adam2392@gmail.com>
Bumps [abatilo/actions-poetry](https://github.com/abatilo/actions-poetry) from 2.0.0 to 2.1.5.
- [Release notes](https://github.com/abatilo/actions-poetry/releases)
- [Changelog](https://github.com/abatilo/actions-poetry/blob/master/.releaserc)
- [Commits](abatilo/actions-poetry@v2.0.0...v2.1.5)

---
updated-dependencies:
- dependency-name: abatilo/actions-poetry
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: Adam Li <adam2392@gmail.com>
… tests (py-why#25)

- Improve type checking for CI tests
- add fingerprint for circleCI

Signed-off-by: Adam Li <adam2392@gmail.com>
Signed-off-by: Adam Li <adam2392@gmail.com>
Signed-off-by: Adam Li <adam2392@gmail.com>
Signed-off-by: Adam Li <adam2392@gmail.com>
Signed-off-by: Adam Li <adam2392@gmail.com>
Signed-off-by: Adam Li <adam2392@gmail.com>
Signed-off-by: Adam Li <adam2392@gmail.com>
Signed-off-by: Adam Li <adam2392@gmail.com>
@adam2392 adam2392 merged commit 940828c into py-why:main Aug 24, 2022
@adam2392 adam2392 deleted the skel branch August 24, 2022 02:11
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.

None yet

3 participants