facebookexperimental / Robyn

Robyn is an experimental, AI/ML-powered and open sourced Marketing Mix Modeling (MMM) package from Meta Marketing Science. Our mission is to democratise modeling knowledge, inspire the industry through innovation, reduce human bias in the modeling process & build a strong open source marketing science community.
https://facebookexperimental.github.io/Robyn/
MIT License
1.08k stars 322 forks source link

Issue when running budget allocator with 0-constraint channels but no 0-coef channels #818

Closed m4x3 closed 9 months ago

m4x3 commented 9 months ago

Issue

The budget allocator does not run correctly for a model if the model has no non-zero coefficients for any channel and some channels are forced to zero via the constraints. --> relevant code here:

In the below code channel_for_allocation is a vector that stores the channels with either 0 coefs (zero_coef_channel) or 0 constraints (zero_coef_channel). These channels are supposed to be ignored for the budget allocation.

This code if (length(zero_coef_channel) > 0) {...} is only executed if there are 0 coef channels. If all channels are non-zero, it does not get executed, even if some channels should be forced to 0 via the constraints.

## Exclude 0 coef and 0 constraint channels for the optimisation
  skip_these <- (channel_constr_low == 0 & channel_constr_up == 0)
  zero_constraint_channel <- mediaSpendSorted[skip_these]
  if (any(skip_these) && !quiet) {
    message("Excluded variables (constrained to 0): ", zero_constraint_channel)
  }
  if (!all(coefSelectorSorted)) {
    zero_coef_channel <- setdiff(names(coefSelectorSorted), mediaSpendSorted[coefSelectorSorted])
    if (!quiet) message(
      "Excluded variables (coefficients are 0): ",
      paste(zero_coef_channel, collapse = ", "))
  } else {
    zero_coef_channel <- as.character()
  }
  channel_for_allocation_loc <- mediaSpendSorted %in% c(zero_coef_channel, zero_constraint_channel)
  channel_for_allocation <- mediaSpendSorted[!channel_for_allocation_loc]
  if (length(zero_coef_channel) > 0) {
    temp_init <- temp_init_all[channel_for_allocation]
    temp_ub <- temp_ub_all[channel_for_allocation]
    temp_lb <- temp_lb_all[channel_for_allocation]
    temp_ub_ext <- temp_ub_ext_all[channel_for_allocation]
    temp_lb_ext <- temp_lb_ext_all[channel_for_allocation]
    x0 <- x0_all[channel_for_allocation]
    lb <- lb_all[channel_for_allocation]
    ub <- ub_all[channel_for_allocation]
    x0_ext <- x0_ext_all[channel_for_allocation]
    lb_ext <- lb_ext_all[channel_for_allocation]
    ub_ext <- ub_ext_all[channel_for_allocation]
  }

Proposed Solution

I think here it should therefore rather be: if (length(zero_coef_channel) > 0 | length(zero_constraint_channel) > 0) {...}

gufengzhou commented 9 months ago

Hi thanks for raising this! So far it looks like you're right. I'll test a bit more and let you know.

gufengzhou commented 9 months ago

just pushed a fix. please reopen if necessary