facebookincubator / GeoLift

GeoLift is an end-to-end geo-experimental methodology based on Synthetic Control Methods used to measure the true incremental effect (Lift) of ad campaign.
https://facebookincubator.github.io/GeoLift/
MIT License
182 stars 55 forks source link

Some potential bugs #63

Closed yulisong closed 2 years ago

yulisong commented 2 years ago

Hi @NicolasMatrices-v2 @ArturoEsquerra,

Thank you so much to you and your team for writing this awesome package! It provides us another choice for geo test. I'm very interested in it and have spent some time learning it. These days, I've found two potential issues of this package. Please correct me if I'm wrong.

1st issue description

Source code of GeoLiftMarketSelection(), line 31 to 32:

N <- append(length(include_markets), N[length(include_markets) <= N])

The code above may generate the length(include_markets) element twice. For instance, N=c(1,2,3,4,5), length(include_markets)=2. It may slow down the computation of GeoLiftMarketSelection() considering it may add another round of for loop.

I think we can use N <- append(length(include_markets), N[length(include_markets) < N]) instead.

2nd issue description

Source code of GeoLiftMarketSelection(), line 50 to 55:

  if (min(effect_size < 0 & max(effect_size) > 0)) {
    message(paste0("Error: The specified simulated effect sizes are not all of the same ", 
      " sign. \nTry again with a vector of all positive or negative effects", 
      " sizes that includes zero."))
    return(NULL)
  }

The code above seems not very meaningful for me. If we want min(effect_size < 0 & max(effect_size) > 0) is true, we need both effect_size < 0 and max(effect_size) > 0 are true. However, if effect_size<0 is true, we can't expect the maximum value of effect_size is greater than 0.

Session information

image

Looking forward to your reply.

Regards, Yuli

NicolasMatrices-v2 commented 2 years ago

Hi Yuli! Thank you very much for all your feedback!! We are happy that GeoLift is helping you run better geographical experiments.

We have just landed the fix related to your comments. Feel free to check the linked PR for it.

Closing this out for now! Feel free to reopen or submit another issue if you have further comments.

Best!