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

Attempt to modularize AC query code #240

Closed
wants to merge 42 commits into from

Conversation

eb8680
Copy link
Contributor

@eb8680 eb8680 commented Aug 31, 2023

Part of modularizing and simplifying #236 #164
Targets #236, not master
Depends on #238 #239

This PR is an attempt at simplifying the query code from @rfl-urbaniak's notebooks and turning it into library code. It attempts to be faithful to the notebooks, although after going through this exercise I have some new questions about the relationships between the textbook definitions and the latest query code in #236.

Only the changes in the new files chirho/counterfactual/handlers/explanation.py and test/counterfactual/test_handlers_explanation.py are new here. The others are merged from #238 and #239 and should land in master relatively soon.

@eb8680 eb8680 added enhancement New feature or request status:awaiting review Awaiting response from reviewer labels Aug 31, 2023
Comment on lines 94 to 99
antecedent_handler = PartOfCause(
antecedents, bias=antecedent_bias, prefix="__antecedent_"
)
treatment_handler = PartOfCause(
treatments, bias=treatment_bias, prefix="__treatment_"
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm a little unsure about this pair of PartOfCause handlers. It's not clear to me whether they're correctly encoding the relevant portion of the textbook definition.

Comment on lines 25 to 26
if antecedents is None:
antecedents_ = list(indices_of(value, event_dim=event_dim).keys())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note the new default behavior of factual_preemption (previously preempt_with_factual) here. This is not exercised in the query code below or the pair of unit tests for PartOfCause initially included with this PR, and may be assuming too much about the free variables of value. I may remove or revert this before a version of factual_preemption lands in master.

@eb8680 eb8680 added status:WIP Work-in-progress not yet ready for review discussion and removed status:awaiting review Awaiting response from reviewer labels Sep 11, 2023
@eb8680
Copy link
Contributor Author

eb8680 commented Oct 9, 2023

Closing this now that PRs for all of the components except ExplainCauses have landed on master.

@eb8680 eb8680 closed this Oct 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion enhancement New feature or request module:explainable status:WIP Work-in-progress not yet ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants