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

Relative Effects incorrect (wrong sign) when post-period mean is negative #31

Open
mefergus opened this issue May 20, 2019 · 4 comments

Comments

@mefergus
Copy link

See impact_inference.R, line 248:

  summary <- dplyr::mutate(summary,
                           RelEffect = AbsEffect / Pred,
                           RelEffect.lower = AbsEffect.lower / Pred,
                           RelEffect.upper = AbsEffect.upper / Pred,
                           RelEffect.sd = AbsEffect.sd / Pred)

I noticed negative RelEffect.sd in my CausalImpactObject$summary which is clearly the product of not using abs(Pred) as the denominator, as I believe it should be in each of these cases. In particular the upper and lower bounds are out of order, too, when Pred < 0 .

@mefergus
Copy link
Author

It may be sufficient to issue a warning when Pred is negative -- further consideration reminds me that working with relative measures "around" zero is fundamentally problematic.

@k3jiang
Copy link

k3jiang commented Sep 19, 2019

Bump I encountered this too.

If the expected impact is negative but the intervention impact is positive, the relative response value stops making sense. For example expected = -0.00041, response = 0.00057, the model outputs -239% when it should be +239%.

image

@vector-news
Copy link

I just want to confirm that this bug is still ongoing and that an implementation of the suggestion by @mefergus does indeed work (i.e. wrap the variable Pred in abs() for line 248). I have forked the package and you can find it here: https://github.com/vector-news/CausalImpact and you can install it using devtools::install_github("vector-news/CausalImpact")

I have also opened a pull request to fix this issue, which you can see here: #60. Thank you @mefergus and others for looking into this issue. Hopefully, it will be resolved for future versions.

@kaybrodersen
Copy link
Collaborator

Hi there, this will be resolved in the upcoming CausalImpact 1.3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants