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

Remove use of CausalModel from notebooks and test files #1220

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

rahulbshrestha
Copy link
Contributor

This PR addresses this issue. Since CausalModel is deprecated, it should be removed from tutorials and test cases.

Signed-off-by: rahulbshrestha <[email protected]>
@rahulbshrestha
Copy link
Contributor Author

@bloebp I did it for one file, could you please let me know if this is fine?

The only thing is when I tried to compare the variables for identify_effect for before and after, it seems to be different? e.g if I do

from dowhy.causal_identifier import identify_effect
identification_1 = identify_effect(nx_graph, action_nodes=causes, outcome_nodes=outcomes, observed_nodes=list(graph.get_all_nodes(include_unobserved=False)))

model = CausalModel(df, causes, outcomes, common_causes=common_causes),
nx_graph = model._graph._graph
identification_2 = model.identify_effect(proceed_when_unidentifiable=True)

identification_1 == identification_2

The output is False, any idea why? I can dig into this a bit more too.

@amit-sharma
Copy link
Member

thanks for starting this @rahulbshrestha
In what way does the two identification objects differ? One difference I see is that you may need to use nx_graph as input to CausalModel too (causalmodel accepts a nx graph as graph)

@amit-sharma amit-sharma requested a review from bloebp July 1, 2024 05:51
@rahulbshrestha
Copy link
Contributor Author

rahulbshrestha commented Jul 2, 2024

@amit-sharma I investigated this a bit more and found the difference between both:

identification_1.identifier returns None

identification_2.identifier returns <dowhy.causal_identifier.auto_identifier.AutoIdentifier at 0x2876e2690>

Is this an intended design choice or a bug?

@bloebp
Copy link
Member

bloebp commented Jul 9, 2024

Hey, I think @amit-sharma is better in answering this. Generally, I would expect that we obtain 1:1 the same. If not, there might be an issue we should investigate.

@amit-sharma
Copy link
Member

@rahulbshrestha yeah you are right! I checked the two implementations of identify_effect. The older 'CausalModelimplementation returns a reference to theCausalIdentifier` class that computed the identification. But this does not apply for the new API since it is function-based.

So we'll need to implement a custom __eq__ method for the IdentifiedEstimand class that only compares all the remaining attributes. Would you be up for adding that?

Copy link

github-actions bot commented Oct 3, 2024

This PR is stale because it has been open for 60 days with no activity.

@github-actions github-actions bot added the stale label Oct 3, 2024
Copy link

This PR was closed because it has been inactive for 7 days since being marked as stale.

@github-actions github-actions bot closed this Oct 17, 2024
@bloebp bloebp reopened this Oct 17, 2024
@bloebp
Copy link
Member

bloebp commented Oct 17, 2024

@amit-sharma any chance to take a look?

@amit-sharma
Copy link
Member

yeah, this PR needs a bit of more work. We'll need to implement __eq__ method as I described above.
@rahulbshrestha are you able to update this PR?

@github-actions github-actions bot removed the stale label Oct 18, 2024
Copy link

This PR is stale because it has been open for 60 days with no activity.

@github-actions github-actions bot added the stale label Dec 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants