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.

Sign up for GitHub

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

Jump to bottom

Bug: Possible to add Edges forward and backwards #85

Merged
merged 7 commits into from
Sep 17, 2024
Merged

Bug: Possible to add Edges forward and backwards #85

merged 7 commits into from
Sep 17, 2024

Conversation

damien-causalens
Copy link
Contributor

@damien-causalens damien-causalens commented Sep 16, 2024 •
edited
Loading

Description and Motivation

Added CausalGraphErrors.ReverseEdgeExistsError Exception, which allows to distinguish acyclicity and double-reverse edges. Introduced a check in the CausalGraph._set_edge() method.

Additional Context

ADS discovered a bug that allowed the addition of directed and undirected edges between b and a to a graph with an existing undirected edge between a and b.
Jira link: CAUSALAI-5495

How Has This Been Tested?

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Improvement (non-breaking change which improves functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Refactor (moving code around, general refactoring improvements)
  • Documentation (documentation)

PR Checklist:

  • I have reviewed my own PR? (review the PR as you are reviewing someone else's PR)
  • I have implemented all requirements? (see JIRA, project documentation)
  • I am affecting someone else's work? If so: I have added those people as reviewers?
  • I have added comments to all the bits that are hard to follow
  • At a bare minimum, I tested this manually
  • I have added unit test(s)
  • Is this a bugfix? If so have I added a regression test?
  • Does this PR satisfy the one PR, one concern rule?
  • If needed, documentation has been added/updated?
  • I have updated the appropriate changelog with a line for my changes
  • If this PR breaks reproducibility, have I added a note in the changelog explaining the break in reproducibility
    and its extent?

Screenshots (if appropriate):

@damien-causalens damien-causalens added the Waiting for review This PR is now ready for the review process, this means tests passed and all deliverables are in. label Sep 16, 2024
@damien-causalens damien-causalens self-assigned this Sep 16, 2024
@damien-causalens damien-causalens requested review from a team as code owners September 16, 2024 14:08
matteo-causalens
matteo-causalens reviewed Sep 16, 2024
View reviewed changes
Copy link
Contributor

@matteo-causalens matteo-causalens left a comment

Choose a reason for hiding this comment

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

Hey @damien-causalens, LGTM. I just had a few minor comments.

changelog.md Outdated Show resolved Hide resolved
tests/test_causal_graph.py Show resolved Hide resolved
cai_causal_graph/causal_graph.py Outdated Show resolved Hide resolved
steven-causalens
steven-causalens reviewed Sep 16, 2024
View reviewed changes
Copy link
Contributor

@steven-causalens steven-causalens left a comment

Choose a reason for hiding this comment

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

Awesome work @damien-causalens 🥳 Overall logic is sound, only had a few comments about phrasing, and one about a small efficiency improvement.

cai_causal_graph/causal_graph.py Outdated Show resolved Hide resolved
cai_causal_graph/causal_graph.py Show resolved Hide resolved
cai_causal_graph/exceptions.py Outdated Show resolved Hide resolved
changelog.md Outdated Show resolved Hide resolved
changelog.md Outdated Show resolved Hide resolved
tests/test_causal_graph.py Show resolved Hide resolved
@steven-causalens steven-causalens changed the title Bug/causalai 5495 edges forward backward possible Bug: Possible to add Edges forward and backwards Sep 17, 2024
steven-causalens
steven-causalens reviewed Sep 17, 2024
View reviewed changes
Copy link
Contributor

@steven-causalens steven-causalens left a comment

Choose a reason for hiding this comment

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

Two small comments in the changelog, will approve once addressed!

changelog.md Outdated Show resolved Hide resolved
changelog.md Outdated Show resolved Hide resolved
steven-causalens
steven-causalens approved these changes Sep 17, 2024
View reviewed changes
Copy link
Contributor

@steven-causalens steven-causalens left a comment

Choose a reason for hiding this comment

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

Amazing work @damien-causalens, LGTM! 🥳

matteo-causalens
matteo-causalens approved these changes Sep 17, 2024
View reviewed changes
Copy link
Contributor

@matteo-causalens matteo-causalens left a comment

Choose a reason for hiding this comment

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

Hey @damien-causalens, great! LGTM.

@steven-causalens steven-causalens merged commit 00e7123 into main Sep 17, 2024
1 check passed
@steven-causalens steven-causalens deleted the bug/CAUSALAI-5495-edges-forward-backward-possible branch September 17, 2024 09:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@steven-causalens steven-causalens steven-causalens approved these changes

@matteo-causalens matteo-causalens matteo-causalens approved these changes

@ilya-causalens ilya-causalens Awaiting requested review from ilya-causalens

Assignees

@damien-causalens damien-causalens

Labels
Waiting for review This PR is now ready for the review process, this means tests passed and all deliverables are in.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@damien-causalens @steven-causalens @matteo-causalens