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

Explainable undo split #562

Merged
merged 57 commits into from
Aug 23, 2024
Merged

Explainable undo split #562

merged 57 commits into from
Aug 23, 2024

Conversation

PoorvaGarg
Copy link
Contributor

@PoorvaGarg PoorvaGarg commented Aug 19, 2024 •
edited
Loading

This pull request resolves the issue #561 by modifying the code for chirho.explainable.handlers.components.undo_split and contributes additional tests as a safeguard.

This specifically resolves inaccuracies for SearchForExplanation where split would create two counterfactual worlds and undo_split would only undo one of them.

PoorvaGarg and others added 30 commits August 1, 2024 17:02
@PoorvaGarg
tests for indepepdent and correct
080e39d
@rfl-urbaniak
added print
7dd59ad
@rfl-urbaniak
extra case
03516b7
@rfl-urbaniak
debugged reverse
a7b3a8a
@PoorvaGarg
debug consequen_eq_neq
6ee8651
@rfl-urbaniak
fixed test_consequent_eq_neq
513ec6e
@rfl-urbaniak
fixed the test with dimensions
ce96b9f
@PoorvaGarg
consequent_eq_neq
1f8e72a
@PoorvaGarg
three variable model
bc5ffb6
@rfl-urbaniak
testing three dependent
5aabfc6
@rfl-urbaniak
debugging
776208f
@PoorvaGarg
minimal example for three independent variables
c0a22c0
@PoorvaGarg
more three variable models
1911de2
@PoorvaGarg
diverge
84381e2
@rfl-urbaniak
debugged
bbc121c
@PoorvaGarg
notebook tested three variable models
498f070
@PoorvaGarg
three variable test cases aded
1e65c7d
@PoorvaGarg
clean up
4058660
@PoorvaGarg
test for factual log probs
34d0faf
@PoorvaGarg
more clean up
9326a52
@PoorvaGarg
fixed a lint error
45e75d6
@PoorvaGarg
lint clean
075c33a
@PoorvaGarg
reverted metadata
dde4d36
@PoorvaGarg
ground truth for conditioning on deterministic node
75e9f05
@PoorvaGarg
responsibility debug
7e2501a
@PoorvaGarg
documentation commit
2a38798
@PoorvaGarg
responsibility example
9254529
@PoorvaGarg
documentation completed
61aa26b
@rfl-urbaniak
small typos
58842f6
@PoorvaGarg
small changes
9ee3068
@PoorvaGarg PoorvaGarg added bug Something isn't working status:WIP Work-in-progress not yet ready for review module:explainable labels Aug 19, 2024
@rfl-urbaniak
Copy link
Collaborator

Looks good to me, @eb8680 can you review please?

@PoorvaGarg PoorvaGarg added status:awaiting review Awaiting response from reviewer and removed status:WIP Work-in-progress not yet ready for review labels Aug 22, 2024
@eb8680 eb8680 marked this pull request as ready for review August 23, 2024 18:45
@eb8680
Copy link
Contributor

eb8680 commented Aug 23, 2024

@PoorvaGarg can you resolve the merge conflicts in the notebook?

@eb8680 eb8680 added status:awaiting response Awaiting response from creator and removed status:awaiting review Awaiting response from reviewer labels Aug 23, 2024
eb8680
eb8680 approved these changes Aug 23, 2024
View reviewed changes
Copy link
Contributor

@eb8680 eb8680 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for fixing this!

@eb8680 eb8680 linked an issue Aug 23, 2024 that may be closed by this pull request
Correctness bug in chirho.explainable.handlers.components.undo_split #561
Closed
@PoorvaGarg PoorvaGarg added status:awaiting review Awaiting response from reviewer and removed status:awaiting response Awaiting response from creator labels Aug 23, 2024
@PoorvaGarg PoorvaGarg merged commit 0553360 into master Aug 23, 2024
7 checks passed
@PoorvaGarg PoorvaGarg deleted the explainable-undo_split branch August 23, 2024 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eb8680 eb8680 eb8680 approved these changes

Assignees
No one assigned
Labels
bug Something isn't working module:explainable status:awaiting review Awaiting response from reviewer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Correctness bug in chirho.explainable.handlers.components.undo_split
3 participants
@PoorvaGarg @rfl-urbaniak @eb8680