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

[DOC] Adding basic conceptual example of causal graphs #11

Merged
merged 18 commits into from
Sep 21, 2022

Conversation

adam2392
Copy link
Collaborator

@adam2392 adam2392 commented Aug 30, 2022

I've been merging in some PRs without reviews for the sake of getting upstream PRs to work. Now I think we're at a stage where we can improve the codebase, but also not be a bottleneck for super common algorithms upstream (e.g. FCI). This PR is now a reviewable PR for the sake of converging on the communication of concepts to end-users.

Addresses partially: #3

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.

Signed-off-by: Adam Li <[email protected]>
Signed-off-by: Adam Li <[email protected]>
Signed-off-by: Adam Li <[email protected]>
Signed-off-by: Adam Li <[email protected]>
Signed-off-by: Adam Li <[email protected]>
Signed-off-by: Adam Li <[email protected]>
Copy link
Member

@petergtz petergtz left a comment

Choose a reason for hiding this comment

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

Hey @adam2392, I've done a very superficial review (given my lack of theoretical background in the science topic) and would expect others to weigh in too.

A general comment: to make these kinds of reviews easier for reviewers, please don't mix different changes into one PR. I think the changes in pag.py and admg.py should not be in this PR (and probably those in pyproject.toml neither).

I know that it's often annoying to fiddle around with different branches, separating different changes into those different branches, etc. But there's one huge benefit I've observed in the past: reviews and especially approvals will happen faster, because there's less to review, and one requested change does not block all the other changes you made. So I'd encourage you to try it out.

examples/intro_causal_graphs.py Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved

# The bidirected edges also form a cluster in what is known as "confounded-components", or
# c-components for short.
print(f"The ADMG has c-components: {list(admg.c_components())}")
Copy link
Member

Choose a reason for hiding this comment

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

Is "c_components" a well-known expression in the science community or would this benefit from the verbose name "confounded_components".

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think the string can be more explicit, but in general anyone working in causality from graphical model perspective would get exposed to c-components (imo), but open to changing the property name too.

examples/intro_causal_graphs.py Outdated Show resolved Hide resolved
pywhy_graphs/algorithms/pag.py Outdated Show resolved Hide resolved
@adam2392
Copy link
Collaborator Author

adam2392 commented Sep 7, 2022

I know that it's often annoying to fiddle around with different branches, separating different changes into those different branches, etc. But there's one huge benefit I've observed in the past: reviews and especially approvals will happen faster, because there's less to review, and one requested change does not block all the other changes you made. So I'd encourage you to try it out.

Hey @petergtz sure I can do that. I figured small changes on top of the main contributing example were fine, but I guess they kind of stacked up to a significant # LOC :/.

I'm okay with waiting on this one to merge in. This is an example really trying to communicate core concepts, and I think in general will be one of the first things a user might look at, so def a higher bar.

I'll try to be more cognizant of breaking up the proposed changes more.

Signed-off-by: Adam Li <[email protected]>
Signed-off-by: Adam Li <[email protected]>
Signed-off-by: Adam Li <[email protected]>
Signed-off-by: Adam Li <[email protected]>
Signed-off-by: Adam Li <[email protected]>
Signed-off-by: Adam Li <[email protected]>
@codecov-commenter
Copy link

Codecov Report

Merging #11 (08d0d50) into main (8c2ff2e) will decrease coverage by 1.10%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main      #11      +/-   ##
==========================================
- Coverage   68.27%   67.16%   -1.11%     
==========================================
  Files          14       14              
  Lines         788      801      +13     
  Branches      208      212       +4     
==========================================
  Hits          538      538              
- Misses        204      217      +13     
  Partials       46       46              
Impacted Files Coverage Δ
pywhy_graphs/viz/draw.py 0.00% <0.00%> (ø)

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

@adam2392 adam2392 merged commit b5bfa64 into py-why:main Sep 21, 2022
@adam2392 adam2392 deleted the example branch September 21, 2022 12:30
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.

4 participants