Ability to allocate traffic for variant by weights #20
Conversation
|
Hi @szymansd. Thanks a lot for your contribution. It looks like an interesting addition to the project. I will try to have a look some time and understand what you're trying to achieve. It might be useful if you could describe it a bit more? or maybe add some comments in the code if it's not too much trouble. It would just help me understand things when reviewing it. I'd be happy to help you get these changes merged, but I want to make sure I understand the changes and can suggest the best way to implement them. (apologies if I'm a bit slow to respond, because things are particularly busy right now, but feel free to remind me if I forget) |
|
Sorry for the delay. I've added additional comments into code. Main changes are in alephbet.coffe file. I've added really simple algorithm. |
There was a problem hiding this comment.
Thanks for adding comments. Overall it looks good, but I think there are a few potential improvements. One particular thing to make sure is that it plays well with user-based tracking.
Would you be able to write tests for this change? I hope the test code is clear enough to extend.
src/alephbet.coffee
Outdated
| # =======^ | ||
| # Select C | ||
|
|
||
| # I'm assuming that all weights will sum up to 100 |
There was a problem hiding this comment.
why make this assumption though? Couldn't you just sum up all weights and then pick the random number between 0 and the total weights?
src/alephbet.coffee
Outdated
|
|
||
| # I'm assuming that all weights will sum up to 100 | ||
| # then I pick random number from 0 - 100 | ||
| weightedIndex = Math.floor(Math.random() * 100) |
There was a problem hiding this comment.
I believe you should use _random instead of Math.random so this plays nicely with user-based tracking
src/alephbet.coffee
Outdated
| @@ -67,6 +67,34 @@ class AlephBet | |||
| @storage().get("#{@options.name}:variant") | |||
|
|
|||
| pick_variant: -> | |||
| # we are checking that all variants of experiment has weights | |||
| variants_has_weight = utils.checkWeights(@variants).every (contains_weight) -> contains_weight | |||
There was a problem hiding this comment.
can this logic move into utils where anyway most of the code that check weights exists?
There was a problem hiding this comment.
... also, it should probably say variants_have_weights, since variants are plural, and maybe clearer would be all_variants_have_weights because you are making sure all of them have weights.
It might also make sense to log a warning if only some have weights, because then it's more likely to be a mistake and the test won't behave as expected.
|
I've updated code. Also I've wrote few tests. Please look at them and justify are they sufficient. |
There was a problem hiding this comment.
Thanks again @szymansd. The code looks much clearer now, and the tests are great! I really appreciate it.
I made some comments. It's mostly a personal style I suppose. But that's something I can apply before merging this. Just to let you know.
I'm not sure however about the + 1 when picking a weighted index...
There are also some slight grammar issues with things like "variants has* weight". I'd be happy to fix those as well. (I'm guessing you're not an native English speaker, so these are common mistakes... I'm not a native English speaker either btw)
src/alephbet.coffee
Outdated
| # then I pick random number from 0 - 100 | ||
| weightedIndex = Math.floor(Math.random() * 100) | ||
| weights_sum = utils.sumWeights(@variants) | ||
| weighted_index = Math.floor((@_random('variant') * 100) + 1 ) |
There was a problem hiding this comment.
this still uses 100 instead of the sum of weights though ... And why + 1 ?
| # =======^ | ||
| # Select C | ||
| weights_sum = utils.sumWeights(@variants) | ||
| weighted_index = Math.floor((@_random('variant') * weights_sum) + 1 ) |
| contains_weight = [] | ||
| contains_weight.push(value.weight?) for key, value of variants | ||
| contains_weight.every (has_weight) -> has_weight | ||
| @sumWeights: (variants) -> |
There was a problem hiding this comment.
I would probably express this as
variants.reduce ((total, variant) -> total + (variant.weight || 0)), 0it's slightly more dense, but it's "just" a sum of weights...
| sum += value.weight if value.weight? | ||
| sum | ||
| @validateWeights: (variants) -> | ||
| contains_weight = [] |
There was a problem hiding this comment.
again, it's a matter of personal preference, but I would find it clearer to write something like
variants_with_weights = variants.filter((variant) -> variant.weight).length
return true if variants_with_weights == 0 || variants.length == variants_with_weights
false(then you can also re-use variants_with_weights as a shared function
| @@ -18,4 +18,17 @@ class Utils | |||
| return Math.random() unless seed | |||
| # a MUCH simplified version inspired by PlanOut.js | |||
| parseInt(@sha1(seed).substr(0, 13), 16) / 0xFFFFFFFFFFFFF | |||
| @checkWeights: (variants) -> | |||
| contains_weight = [] | |||
There was a problem hiding this comment.
I would personally write this as
variants_with_weights = variants.filter((variant) -> variant.weight).length
variants_with_weights == variants.length
|
@szymansd thanks again for the PR. I pushed some changes on top of yours. Can you have a look and see if it's working as expected for you? then we can properly release it :) |
|
@gingerlime thanks for changes. Code looks much better now. Everything is working as expected so you are go to go 👍 Sorry for the delay - I totally forgot about this PR. |
|
Thanks again @szymansd. I just released v0.17.0 with your feature :) |
I've implemented allocating users mechanism. I would like to start discussion about this. It's not perfect but should work pretty well. If you have any questions please do not hesitate to ask. I'm a JS developer so my code in Cofeescript may be far from perfect so all ideas for improvements are welcome. Right now the only missing part is readme file and tests so before accepting this PR please contact me and I will provide it.