CausalModel.estimate_effect - UnboundLocalError: local variable 'identifier_name' referenced before assignment

[leo-ware]

Hi guys,

I'm running into an issue with CausalModel.estimate_effect. It looks like when you don't pass method_name, the local variable identifier_name doesn't get initialized, and it throws an UnboundLocalError for identifier_name on line 279 when its trying to figure out how to initialize the CausalEstimate.

This is kind of an unhelpful error message because identifier_name is internal to the method, so you have to open up the source to figure out what's going on. And, it will always throw if method_name is None and identified_estimand.no_directed_path is truthy.

A minimal fix would be to check if it's going to throw and preemptively throw something else (a ValueError?) with a helpful error message explaining what's going on. A more sustainable fix would be to implements defaults (there's a todo comment explaining how to approach this.)

if method_name is None:
        #TODO add propensity score as default backdoor method, iv as default iv method, add an informational message to show which method has been selected.
        pass

I'll do the second option and implement the defaults if you guys want me to. Should I do this?

[amit-sharma]

That's a good catch, @leo-ware. Yes, the code should ideally return a ValueError rather than simply continuing.

The default option sounds great. Will be great if you can implement it. That inline comment is a bit dated, here's how I am thinking:

• the identified_estimand has a list of estimands, including backdoor, iv, etc.
• If a backdoor estimand is available, let's make backdoor the default (since it is the most commonly used)
  • If the treatment is binary, then default is propensity score stratification.
  • If the treatment is continuous, then default is linear regression.
• If no backdoor estimand is available, then check if IV exists.
  • If IV exists, then default is the IV estimator.
• If none of these exist, we can probably throw a ValueError. There are other identification strategies like frontdoor, but they are currently experimental in dowhy and it may be too early to add them as defaults.

What do you think? Does this make sense for the implementation?

[leo-ware]

Sounds good. I will get started.

[leo-ware]

Just submitted a pull request for this.

[amit-sharma]

Thanks, there is a CLA sign pending. Can you have a look at that?

[leo-ware]

done

[EgorKraevTransferwise]

Please, @amit-sharma could you provide a link to the PR? From what I can see, the line in question is still there. Also, for the case when both iv and backdoor have been identified, why would we choose backdoor?

[leo-ware]

@EgorKraevTransferwise Thanks for reminding me, I had completely forgotten about this :) There was an issue with my pull request, and I got pulled away to another project before I could fix it. Give me a couple days and I will get it back up.

[leo-ware]

ok actually i've been super busy with coursework, and i haven't been able to tackle this. if someone else wants to take it on i'd be happy with that.