search

klarsen1 / MarketMatching

Other
131 stars 37 forks source link

Specify id variable as factor in PlotActuals #16

Closed paulbeckmann closed 4 years ago

paulbeckmann commented 5 years ago

Hi there! I frequently work with the MarketMatching package (and I love it!). One (minor) bug I frequently come across is that numeric id variables result in a wrongly specified plot as outcome of the inference function. In the code that generates the PlotActuals plot, this bug could easily be fixed by specifying the color variable as a factor variable right in the plot. (Of course, it is also possible to specify the id variable as factor variable earlier on but in my experience this often leads to other errors such as an incorrect assignment of factor levels to id variables). The following tiny change to line 585 and below should do the trick:

  plotdf <- data[data$id_var %in% c(test_market, control_market),]
  results[[12]] <- ggplot(data=plotdf, aes(x=date_var, y=match_var, colour=as.factor(id_var))) +
    geom_line() +
    theme_bw() + theme(legend.title = element_blank(), axis.title.x = element_blank()) + ylab("") + xlab("Date") +
    geom_vline(xintercept=as.numeric(MatchingEndDate), linetype=2) +
    scale_y_continuous(labels = scales::comma, limits=c(ymin, ymax))`

Would it be possible to implement this change in the next version of MarketMatching?

klarsen1 commented 5 years ago

Yes, for sure.

On Fri, Oct 11, 2019 at 5:46 AM paulbeckmann notifications@github.com wrote:

Hi there! I frequently work with the MarketMatching package (and I love it!). One (minor) bug I frequently come across is that numeric id variables result in a wrongly specified plot as outcome of the inference function. In the code that generates the PlotActuals plot, this bug could easily be fixed by specifying the color variable as a factor variable right in the plot. (Of course, it is also possible to specify the id variable as factor variable earlier on but in my experience this often leads to other errors such as an incorrect assignment of factor levels to id variables). The following tiny change to line 585 and below should do the trick:

plotdf <- data[data$id_var %in% c(test_market, control_market),] results[[12]] <- ggplot(data=plotdf, aes(x=date_var, y=match_var, colour=as.factor(id_var))) + geom_line() + theme_bw() + theme(legend.title = element_blank(), axis.title.x = element_blank()) + ylab("") + xlab("Date") + geom_vline(xintercept=as.numeric(MatchingEndDate), linetype=2) + scale_y_continuous(labels = scales::comma, limits=c(ymin, ymax))`

Would it be possible to implement this change in the next version of MarketMatching?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/klarsen1/MarketMatching/issues/16?email_source=notifications&email_token=ACNKU5HDXNFSWVMYIGJVUJTQOBYT5A5CNFSM4I7ZKQUKYY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4HRGMENA, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACNKU5CZYXYRJ2P4APSAXX3QOBYT5ANCNFSM4I7ZKQUA .

klarsen1 commented 4 years ago

This change is in GitHub now (not CRAN yet)