From fcda139b0c4729821b40ccad428deeb0f2ed3fc3 Mon Sep 17 00:00:00 2001 From: Rowan441 Date: Tue, 26 Apr 2022 16:13:52 -0400 Subject: [PATCH 1/4] add new algorithm for systematic sampling --- lib/split.rb | 1 + lib/split/algorithms/systematic_sampling.rb | 18 ++++++ lib/split/configuration.rb | 3 +- lib/split/experiment.rb | 16 ++++++ spec/algorithms/systematic_sampling_spec.rb | 62 +++++++++++++++++++++ spec/experiment_spec.rb | 15 +++++ 6 files changed, 114 insertions(+), 1 deletion(-) create mode 100644 lib/split/algorithms/systematic_sampling.rb create mode 100644 spec/algorithms/systematic_sampling_spec.rb diff --git a/lib/split.rb b/lib/split.rb index 176023fa..667ba1e1 100755 --- a/lib/split.rb +++ b/lib/split.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true require 'redis' +require 'split/algorithms/systematic_sampling' require 'split/algorithms/block_randomization' require 'split/algorithms/weighted_sample' require 'split/algorithms/whiplash' diff --git a/lib/split/algorithms/systematic_sampling.rb b/lib/split/algorithms/systematic_sampling.rb new file mode 100644 index 00000000..9ab8e8ec --- /dev/null +++ b/lib/split/algorithms/systematic_sampling.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true +module Split + module Algorithms + module SystematicSampling + def self.choose_alternative(experiment) + block = experiment.cohorting_block + raise ArgumentError, "Experiment configuration is missing cohorting_block array" unless block + index = experiment.participant_count % block.length + chosen_alternative = block[index] + alt = experiment.alternatives.find do |alt| + alt.name == chosen_alternative + end + raise ArgumentError, "Invalid cohorting_block: '#{chosen_alternative}' is not an experiment alternative" unless alt + alt + end + end + end +end diff --git a/lib/split/configuration.rb b/lib/split/configuration.rb index f70b1a66..3ad6c4ec 100644 --- a/lib/split/configuration.rb +++ b/lib/split/configuration.rb @@ -164,7 +164,8 @@ def normalized_experiments algorithm: value_for(settings, :algorithm), resettable: value_for(settings, :resettable), friendly_name: value_for(settings, :friendly_name), - retain_user_alternatives_after_reset: value_for(settings, :retain_user_alternatives_after_reset) + retain_user_alternatives_after_reset: value_for(settings, :retain_user_alternatives_after_reset), + cohorting_block: value_for(settings, :cohorting_block) } experiment_data.each do |name, value| diff --git a/lib/split/experiment.rb b/lib/split/experiment.rb index befdf3d5..e4e9736b 100644 --- a/lib/split/experiment.rb +++ b/lib/split/experiment.rb @@ -9,6 +9,7 @@ class Experiment attr_accessor :alternative_probabilities attr_accessor :metadata attr_accessor :friendly_name + attr_accessor :cohorting_block attr_reader :alternatives attr_reader :resettable @@ -17,6 +18,7 @@ class Experiment DEFAULT_OPTIONS = { :resettable => true, :retain_user_alternatives_after_reset => false, + :cohorting_block => ["control", "alternative"] } def initialize(name, options = {}) @@ -43,6 +45,7 @@ def set_alternatives_and_options(options) self.metadata = options_with_defaults[:metadata] self.friendly_name = options_with_defaults[:friendly_name] || @name self.retain_user_alternatives_after_reset = options_with_defaults[:retain_user_alternatives_after_reset] + self.cohorting_block = options_with_defaults[:cohorting_block] end def extract_alternatives_from_options(options) @@ -64,6 +67,7 @@ def extract_alternatives_from_options(options) options[:algorithm] = exp_config[:algorithm] options[:friendly_name] = exp_config[:friendly_name] options[:retain_user_alternatives_after_reset] = exp_config[:retain_user_alternatives_after_reset] + options[:cohorting_block] = exp_config[:cohorting_block] end end @@ -232,6 +236,10 @@ def friendly_name_key "#{name}:friendly_name" end + def cohorting_block_key + "#{name}:cohorting_block" + end + def resettable? resettable end @@ -266,6 +274,7 @@ def load_from_redis options = { retain_user_alternatives_after_reset: exp_config['retain_user_alternatives_after_reset'], + cohorting_block: load_cohorting_block_from_redis, resettable: exp_config['resettable'], algorithm: exp_config['algorithm'], friendly_name: load_friendly_name_from_redis, @@ -446,6 +455,10 @@ def load_friendly_name_from_redis redis.get(friendly_name_key) end + def load_cohorting_block_from_redis + redis.lrange(cohorting_block_key, 0, -1) + end + def load_alternatives_from_configuration alts = Split.configuration.experiment_for(@name)[:alternatives] raise ArgumentError, "Experiment configuration is missing :alternatives array" unless alts @@ -492,6 +505,9 @@ def persist_experiment_configuration goals_collection.save redis.set(metadata_key, @metadata.to_json) unless @metadata.nil? redis.set(friendly_name_key, self.friendly_name) + self.cohorting_block.each do |entry| + redis.lpush(cohorting_block_key, entry) + end end def remove_experiment_configuration diff --git a/spec/algorithms/systematic_sampling_spec.rb b/spec/algorithms/systematic_sampling_spec.rb new file mode 100644 index 00000000..5f3016bb --- /dev/null +++ b/spec/algorithms/systematic_sampling_spec.rb @@ -0,0 +1,62 @@ +# frozen_string_literal: true +require "spec_helper" + +describe Split::Algorithms::SystematicSampling do + # it "should return an alternative" do + # experiment = Split::ExperimentCatalog.find_or_create('link_color', {'blue' => 100}, {'red' => 0 }) + # expect(Split::Algorithms::WeightedSample.choose_alternative(experiment).class).to eq(Split::Alternative) + # end + + # it "should always return a heavily weighted option" do + # experiment = Split::ExperimentCatalog.find_or_create('link_color', {'blue' => 100}, {'red' => 0 }) + # expect(Split::Algorithms::WeightedSample.choose_alternative(experiment).name).to eq('blue') + # end + + context "for a valid experiment" do + let!(:valid_experiment) do + Split::Experiment.new('link_color', :alternatives => ['red', 'blue', 'green'], :cohorting_block => ['red', 'blue', 'green']) + end + + let(:red_alternative) { Split::Alternative.new('red', 'link_color') } + + it "cohorts the first user into the first alternative defined in cohorting_block" do + expect(Split::Algorithms::SystematicSampling.choose_alternative(valid_experiment).name).to equal "red" + end + + it "cohorts the second user into the second alternative defined in cohorting_block" do + red_alternative.increment_participation + + expect(Split::Algorithms::SystematicSampling.choose_alternative(valid_experiment).name).to equal "blue" + end + + it "cohorts the fourth user into the first alternative defined in cohorting_block" do + red_alternative.increment_participation + red_alternative.increment_participation + red_alternative.increment_participation + + expect(Split::Algorithms::SystematicSampling.choose_alternative(valid_experiment).name).to equal "red" + end + end + + context "for an experiment with no cohorting_block defined" do + let!(:missing_config_experiment) do + Split::Experiment.new('link_color', :alternatives => ['red', 'blue', 'green']) + end + + it "Throws argument error with descriptive message" do + expect { Split::Algorithms::SystematicSampling.choose_alternative(missing_config_experiment).name } + .to raise_error(ArgumentError, "Experiment configuration is missing cohorting_block array") + end + end + + context "for an experiment with invalid cohorting_block defined" do + let!(:invalid_config_experiment) do + Split::Experiment.new('link_color', :alternatives => ['red', 'blue', 'green'], :cohorting_block => ['notarealalternative', 'blue', 'green']) + end + + it "Throws argument error with descriptive message" do + expect { Split::Algorithms::SystematicSampling.choose_alternative(invalid_config_experiment).name } + .to raise_error(ArgumentError, "Invalid cohorting_block: 'notarealalternative' is not an experiment alternative") + end + end +end diff --git a/spec/experiment_spec.rb b/spec/experiment_spec.rb index 80a5b42e..0908dc60 100644 --- a/spec/experiment_spec.rb +++ b/spec/experiment_spec.rb @@ -128,6 +128,11 @@ def alternative(color) expect(experiment.retain_user_alternatives_after_reset).to be_truthy end + it "should be possible to make an experiment with a cohorting_block" do + experiment = Split::Experiment.new("basket_text", :alternatives => ["Basket", "Cart"], :cohorting_block => ["Basket", "Cart"]) + expect(experiment.cohorting_block).to eq(["Basket", "Cart"]) + end + it "sets friendly_name" do experiment = Split::Experiment.new('basket_text', :alternatives => ['Basket', "Cart"], :friendly_name => "foo") expect(experiment.friendly_name).to eq("foo") @@ -149,6 +154,7 @@ def alternative(color) it 'assigns default values to the experiment' do expect(Split::Experiment.new(experiment_name).resettable).to eq(true) expect(Split::Experiment.new(experiment_name).retain_user_alternatives_after_reset).to eq(false) + expect(Split::Experiment.new(experiment_name).cohorting_block).to match_array ['control', 'alternative'] end it "sets friendly_name" do @@ -185,6 +191,15 @@ def alternative(color) expect(e.retain_user_alternatives_after_reset).to be_truthy end + it "should persist cohorting_block" do + experiment = Split::Experiment.new("basket_text", :alternatives => ['Basket', "Cart"], :cohorting_block => ["Basket", "Cart"]) + experiment.save + + e = Split::ExperimentCatalog.find("basket_text") + expect(e).to eq(experiment) + expect(e.cohorting_block).to match_array ["Basket", "Cart"] + end + describe '#metadata' do let(:experiment) { Split::Experiment.new('basket_text', :alternatives => ['Basket', "Cart"], :algorithm => Split::Algorithms::Whiplash, :metadata => meta) } context 'simple hash' do From 27fc32fb6fc02f53b436e341230ab02655cb57a8 Mon Sep 17 00:00:00 2001 From: Rowan441 Date: Thu, 28 Apr 2022 16:57:42 -0400 Subject: [PATCH 2/4] add new algorithm for systematic sampling --- lib/split/algorithms/systematic_sampling.rb | 17 ++-- lib/split/configuration.rb | 3 +- lib/split/experiment.rb | 42 ++++++--- spec/algorithms/systematic_sampling_spec.rb | 97 ++++++++++++--------- spec/configuration_spec.rb | 8 +- spec/experiment_spec.rb | 56 ++++++++++-- 6 files changed, 151 insertions(+), 72 deletions(-) diff --git a/lib/split/algorithms/systematic_sampling.rb b/lib/split/algorithms/systematic_sampling.rb index 9ab8e8ec..f865a2b3 100644 --- a/lib/split/algorithms/systematic_sampling.rb +++ b/lib/split/algorithms/systematic_sampling.rb @@ -3,15 +3,14 @@ module Split module Algorithms module SystematicSampling def self.choose_alternative(experiment) - block = experiment.cohorting_block - raise ArgumentError, "Experiment configuration is missing cohorting_block array" unless block - index = experiment.participant_count % block.length - chosen_alternative = block[index] - alt = experiment.alternatives.find do |alt| - alt.name == chosen_alternative - end - raise ArgumentError, "Invalid cohorting_block: '#{chosen_alternative}' is not an experiment alternative" unless alt - alt + count = experiment.next_cohorting_block_index + + block_length = experiment.cohorting_block_magnitude * experiment.alternatives.length + block_num, index = count.divmod block_length + + r = Random.new(block_num + experiment.cohorting_block_seed) + block = (experiment.alternatives*experiment.cohorting_block_magnitude).shuffle(random: r) + block[index] end end end diff --git a/lib/split/configuration.rb b/lib/split/configuration.rb index 3ad6c4ec..28e7097f 100644 --- a/lib/split/configuration.rb +++ b/lib/split/configuration.rb @@ -165,7 +165,8 @@ def normalized_experiments resettable: value_for(settings, :resettable), friendly_name: value_for(settings, :friendly_name), retain_user_alternatives_after_reset: value_for(settings, :retain_user_alternatives_after_reset), - cohorting_block: value_for(settings, :cohorting_block) + cohorting_block_seed: value_for(settings, :cohorting_block_seed), + cohorting_block_magnitude: value_for(settings, :cohorting_block_magnitude) } experiment_data.each do |name, value| diff --git a/lib/split/experiment.rb b/lib/split/experiment.rb index e4e9736b..2cadaec9 100644 --- a/lib/split/experiment.rb +++ b/lib/split/experiment.rb @@ -9,7 +9,8 @@ class Experiment attr_accessor :alternative_probabilities attr_accessor :metadata attr_accessor :friendly_name - attr_accessor :cohorting_block + attr_accessor :cohorting_block_seed + attr_accessor :cohorting_block_magnitude attr_reader :alternatives attr_reader :resettable @@ -18,7 +19,7 @@ class Experiment DEFAULT_OPTIONS = { :resettable => true, :retain_user_alternatives_after_reset => false, - :cohorting_block => ["control", "alternative"] + :cohorting_block_magnitude => 1 } def initialize(name, options = {}) @@ -45,7 +46,11 @@ def set_alternatives_and_options(options) self.metadata = options_with_defaults[:metadata] self.friendly_name = options_with_defaults[:friendly_name] || @name self.retain_user_alternatives_after_reset = options_with_defaults[:retain_user_alternatives_after_reset] - self.cohorting_block = options_with_defaults[:cohorting_block] + + if self.algorithm == Split::Algorithms::SystematicSampling + self.cohorting_block_seed = options_with_defaults[:cohorting_block_seed] || self.name.to_i(36) + self.cohorting_block_magnitude = options_with_defaults[:cohorting_block_magnitude] + end end def extract_alternatives_from_options(options) @@ -67,7 +72,8 @@ def extract_alternatives_from_options(options) options[:algorithm] = exp_config[:algorithm] options[:friendly_name] = exp_config[:friendly_name] options[:retain_user_alternatives_after_reset] = exp_config[:retain_user_alternatives_after_reset] - options[:cohorting_block] = exp_config[:cohorting_block] + options[:cohorting_block_seed] = exp_config[:cohorting_block_seed] + options[:cohorting_block_magnitude] = exp_config[:cohorting_block_magnitude] end end @@ -236,8 +242,12 @@ def friendly_name_key "#{name}:friendly_name" end - def cohorting_block_key - "#{name}:cohorting_block" + def cohorting_block_seed_key + "#{name}:cohorting_block_seed" + end + + def cohorting_block_magnitude_key + "#{name}:cohorting_block_magnitude" end def resettable? @@ -274,7 +284,8 @@ def load_from_redis options = { retain_user_alternatives_after_reset: exp_config['retain_user_alternatives_after_reset'], - cohorting_block: load_cohorting_block_from_redis, + cohorting_block_seed: load_cohorting_block_seed_from_redis, + cohorting_block_magnitude: load_cohorting_block_magnitude_from_redis, resettable: exp_config['resettable'], algorithm: exp_config['algorithm'], friendly_name: load_friendly_name_from_redis, @@ -432,6 +443,10 @@ def enable_cohorting redis.hset(experiment_config_key, :cohorting, false) end + def next_cohorting_block_index + Split.redis.incr("#{name}:cohorting_block_index") - 1 + end + protected def experiment_config_key @@ -455,8 +470,12 @@ def load_friendly_name_from_redis redis.get(friendly_name_key) end - def load_cohorting_block_from_redis - redis.lrange(cohorting_block_key, 0, -1) + def load_cohorting_block_seed_from_redis + redis.get(cohorting_block_seed_key).to_i + end + + def load_cohorting_block_magnitude_from_redis + redis.get(cohorting_block_magnitude_key).to_i end def load_alternatives_from_configuration @@ -505,9 +524,8 @@ def persist_experiment_configuration goals_collection.save redis.set(metadata_key, @metadata.to_json) unless @metadata.nil? redis.set(friendly_name_key, self.friendly_name) - self.cohorting_block.each do |entry| - redis.lpush(cohorting_block_key, entry) - end + redis.set(cohorting_block_seed_key, self.cohorting_block_seed) + redis.set(cohorting_block_magnitude_key, self.cohorting_block_magnitude) end def remove_experiment_configuration diff --git a/spec/algorithms/systematic_sampling_spec.rb b/spec/algorithms/systematic_sampling_spec.rb index 5f3016bb..a16d88d6 100644 --- a/spec/algorithms/systematic_sampling_spec.rb +++ b/spec/algorithms/systematic_sampling_spec.rb @@ -2,61 +2,76 @@ require "spec_helper" describe Split::Algorithms::SystematicSampling do - # it "should return an alternative" do - # experiment = Split::ExperimentCatalog.find_or_create('link_color', {'blue' => 100}, {'red' => 0 }) - # expect(Split::Algorithms::WeightedSample.choose_alternative(experiment).class).to eq(Split::Alternative) - # end - - # it "should always return a heavily weighted option" do - # experiment = Split::ExperimentCatalog.find_or_create('link_color', {'blue' => 100}, {'red' => 0 }) - # expect(Split::Algorithms::WeightedSample.choose_alternative(experiment).name).to eq('blue') - # end - - context "for a valid experiment" do - let!(:valid_experiment) do - Split::Experiment.new('link_color', :alternatives => ['red', 'blue', 'green'], :cohorting_block => ['red', 'blue', 'green']) - end - - let(:red_alternative) { Split::Alternative.new('red', 'link_color') } + let(:experiment) do + Split::Experiment.new( + 'link_color', + :alternatives => ['red', 'blue', 'green'], + :algorithm => Split::Algorithms::SystematicSampling, + :cohorting_block_magnitude => 2 + ) + end - it "cohorts the first user into the first alternative defined in cohorting_block" do - expect(Split::Algorithms::SystematicSampling.choose_alternative(valid_experiment).name).to equal "red" - end + it "should return an alternative" do + expect(Split::Algorithms::SystematicSampling.choose_alternative(experiment).class).to eq(Split::Alternative) + end - it "cohorts the second user into the second alternative defined in cohorting_block" do - red_alternative.increment_participation + context "experiments with a random seed" do + it "cohorts the first block of users equally into each alternative" do + results = {'red' => 0, 'blue' => 0, 'green' => 0} + 6.times do + results[Split::Algorithms::SystematicSampling.choose_alternative(experiment).name] += 1 + end - expect(Split::Algorithms::SystematicSampling.choose_alternative(valid_experiment).name).to equal "blue" + expect(experiment.cohorting_block_magnitude * experiment.alternatives.length).to eq(6) + expect(results).to eq({'red' => 2, 'blue' => 2, 'green' => 2}) end - it "cohorts the fourth user into the first alternative defined in cohorting_block" do - red_alternative.increment_participation - red_alternative.increment_participation - red_alternative.increment_participation + it "cohorts the second block of users equally into each alternative" do + 6.times do + Split::Algorithms::SystematicSampling.choose_alternative(experiment).name + end + + results = {'red' => 0, 'blue' => 0, 'green' => 0} + 6.times do + results[Split::Algorithms::SystematicSampling.choose_alternative(experiment).name] += 1 + end - expect(Split::Algorithms::SystematicSampling.choose_alternative(valid_experiment).name).to equal "red" + expect(experiment.cohorting_block_magnitude * experiment.alternatives.length).to eq(6) + expect(results).to eq({'red' => 2, 'blue' => 2, 'green' => 2}) end end - context "for an experiment with no cohorting_block defined" do - let!(:missing_config_experiment) do - Split::Experiment.new('link_color', :alternatives => ['red', 'blue', 'green']) + context "experiments with set seed" do + let(:seeded_experiment1) do + Split::Experiment.new( + 'link_color', + :alternatives => ['red', 'blue', 'green'], + :algorithm => Split::Algorithms::SystematicSampling, + :cohorting_block_seed => 1234 + ) end - it "Throws argument error with descriptive message" do - expect { Split::Algorithms::SystematicSampling.choose_alternative(missing_config_experiment).name } - .to raise_error(ArgumentError, "Experiment configuration is missing cohorting_block array") + let(:seeded_experiment2) do + Split::Experiment.new('link_highlight', + :alternatives => ['red', 'blue', 'green'], + :algorithm => Split::Algorithms::SystematicSampling, + :cohorting_block_seed => 1234) end - end - context "for an experiment with invalid cohorting_block defined" do - let!(:invalid_config_experiment) do - Split::Experiment.new('link_color', :alternatives => ['red', 'blue', 'green'], :cohorting_block => ['notarealalternative', 'blue', 'green']) - end + it "cohorts users in a set order" do + results1 = [] + results2 = [] + + 12.times do + results1 << Split::Algorithms::SystematicSampling.choose_alternative(seeded_experiment1).name + end + + 12.times do + results2 << Split::Algorithms::SystematicSampling.choose_alternative(seeded_experiment2).name + end - it "Throws argument error with descriptive message" do - expect { Split::Algorithms::SystematicSampling.choose_alternative(invalid_config_experiment).name } - .to raise_error(ArgumentError, "Invalid cohorting_block: 'notarealalternative' is not an experiment alternative") + expect(seeded_experiment1.cohorting_block_seed).to eq(seeded_experiment2.cohorting_block_seed) + expect(results1).to eq(results2) end end end diff --git a/spec/configuration_spec.rb b/spec/configuration_spec.rb index bff3002f..0b589fe6 100644 --- a/spec/configuration_spec.rb +++ b/spec/configuration_spec.rb @@ -135,6 +135,9 @@ percent: 23 resettable: false retain_user_alternatives_after_reset: true + algorithm: Split::Algorithms::SystematicSampling + cohorting_block_seed: 999 + cohorting_block_magnitude: 3 metric: my_metric another_experiment: alternatives: @@ -145,8 +148,9 @@ end it "should normalize experiments" do - expect(@config.normalized_experiments).to eq({:my_experiment=>{:resettable=>false,:retain_user_alternatives_after_reset=>true,:alternatives=>[{"Control Opt"=>0.67}, - [{"Alt One"=>0.1}, {"Alt Two"=>0.23}]]}, :another_experiment=>{:alternatives=>["a", ["b"]]}}) + expect(@config.normalized_experiments).to eq({:my_experiment=>{:resettable=>false,:retain_user_alternatives_after_reset=>true, :cohorting_block_magnitude=>3, + :algorithm=>"Split::Algorithms::SystematicSampling", :cohorting_block_seed=>999,:alternatives=>[{"Control Opt"=>0.67},[{"Alt One"=>0.1}, {"Alt Two"=>0.23}]]}, + :another_experiment=>{:alternatives=>["a", ["b"]]}}) end it "should recognize metrics" do diff --git a/spec/experiment_spec.rb b/spec/experiment_spec.rb index 0908dc60..db1dce91 100644 --- a/spec/experiment_spec.rb +++ b/spec/experiment_spec.rb @@ -128,9 +128,14 @@ def alternative(color) expect(experiment.retain_user_alternatives_after_reset).to be_truthy end - it "should be possible to make an experiment with a cohorting_block" do - experiment = Split::Experiment.new("basket_text", :alternatives => ["Basket", "Cart"], :cohorting_block => ["Basket", "Cart"]) - expect(experiment.cohorting_block).to eq(["Basket", "Cart"]) + it "should be possible to make a SystematicSampling algorithm experiment with a seeded cohorting block" do + experiment = Split::Experiment.new("basket_text", :alternatives => ["Basket", "Cart"], algorithm: Split::Algorithms::SystematicSampling, :cohorting_block_seed => 123) + expect(experiment.cohorting_block_seed).to eq(123) + end + + it "should be possible to make a SystematicSampling algorithm experiment with a custom cohorting block magnitude" do + experiment = Split::Experiment.new("basket_text", :alternatives => ["Basket", "Cart"], algorithm: Split::Algorithms::SystematicSampling, :cohorting_block_magnitude => 4) + expect(experiment.cohorting_block_magnitude).to eq(4) end it "sets friendly_name" do @@ -154,7 +159,18 @@ def alternative(color) it 'assigns default values to the experiment' do expect(Split::Experiment.new(experiment_name).resettable).to eq(true) expect(Split::Experiment.new(experiment_name).retain_user_alternatives_after_reset).to eq(false) - expect(Split::Experiment.new(experiment_name).cohorting_block).to match_array ['control', 'alternative'] + end + + it 'when the experiment is using SystematicSampling algorithm, assigns cohorting_block_magnitude to a default value' do + expect(Split::Experiment.new('systematic_sampling_exp', algorithm: Split::Algorithms::SystematicSampling).cohorting_block_magnitude).to eq(1) + end + + it 'when the experiment is using SystematicSampling algorithm, assigns cohorting_block_seed to a default value' do + expect(Split::Experiment.new('systematic_sampling_exp', algorithm: Split::Algorithms::SystematicSampling).cohorting_block_seed) + .to eq(387211932128858527590062598078109) + + expect(Split::Experiment.new('systematic_sampling_exp2', algorithm: Split::Algorithms::SystematicSampling).cohorting_block_seed) + .to eq(13939629556638906993242253530811926) end it "sets friendly_name" do @@ -191,13 +207,22 @@ def alternative(color) expect(e.retain_user_alternatives_after_reset).to be_truthy end - it "should persist cohorting_block" do - experiment = Split::Experiment.new("basket_text", :alternatives => ['Basket', "Cart"], :cohorting_block => ["Basket", "Cart"]) + it "should persist cohorting_block_magnitude" do + experiment = Split::Experiment.new("basket_text", :alternatives => ['Basket', "Cart"], algorithm: Split::Algorithms::SystematicSampling, :cohorting_block_magnitude => 2) + experiment.save + + e = Split::ExperimentCatalog.find("basket_text") + expect(e).to eq(experiment) + expect(e.cohorting_block_magnitude).to eq(2) + end + + it "should persist cohorting_block_seed" do + experiment = Split::Experiment.new("basket_text", :alternatives => ['Basket', "Cart"], algorithm: Split::Algorithms::SystematicSampling, :cohorting_block_seed => 12345) experiment.save e = Split::ExperimentCatalog.find("basket_text") expect(e).to eq(experiment) - expect(e.cohorting_block).to match_array ["Basket", "Cart"] + expect(e.cohorting_block_seed).to eq(12345) end describe '#metadata' do @@ -442,6 +467,23 @@ def alternative(color) end end + describe '#next_cohorting_block_index' do + it 'increments each call and starts at 0' do + expect(experiment.next_cohorting_block_index).to eq(0) + expect(experiment.next_cohorting_block_index).to eq(1) + expect(experiment.next_cohorting_block_index).to eq(2) + expect(experiment.next_cohorting_block_index).to eq(3) + end + + it 'persists value' do + expect(experiment.next_cohorting_block_index).to eq(0) + experiment.save + + e = Split::ExperimentCatalog.find("link_color") + expect(e.next_cohorting_block_index).to eq(1) + end + end + describe '#next_alternative' do context 'with multiple alternatives' do let(:experiment) { Split::ExperimentCatalog.find_or_create('link_color', 'blue', 'red', 'green') } From 9786bd95e0e40fec5bbd8c340d5bdb085c5756c5 Mon Sep 17 00:00:00 2001 From: Rowan441 Date: Fri, 29 Apr 2022 10:13:55 -0400 Subject: [PATCH 3/4] implemented review comments --- lib/split/algorithms/systematic_sampling.rb | 10 ++++++++-- lib/split/experiment.rb | 6 +++--- spec/algorithms/systematic_sampling_spec.rb | 5 ++--- spec/experiment_spec.rb | 18 +++++++++--------- 4 files changed, 22 insertions(+), 17 deletions(-) diff --git a/lib/split/algorithms/systematic_sampling.rb b/lib/split/algorithms/systematic_sampling.rb index f865a2b3..46c4a843 100644 --- a/lib/split/algorithms/systematic_sampling.rb +++ b/lib/split/algorithms/systematic_sampling.rb @@ -3,14 +3,20 @@ module Split module Algorithms module SystematicSampling def self.choose_alternative(experiment) - count = experiment.next_cohorting_block_index + count = experiment.next_cohorting_block_count block_length = experiment.cohorting_block_magnitude * experiment.alternatives.length block_num, index = count.divmod block_length + block = generate_block(block_num, experiment) + block[index] + end + + private + + def self.generate_block(block_num, experiment) r = Random.new(block_num + experiment.cohorting_block_seed) block = (experiment.alternatives*experiment.cohorting_block_magnitude).shuffle(random: r) - block[index] end end end diff --git a/lib/split/experiment.rb b/lib/split/experiment.rb index 2cadaec9..405ebdf3 100644 --- a/lib/split/experiment.rb +++ b/lib/split/experiment.rb @@ -48,7 +48,7 @@ def set_alternatives_and_options(options) self.retain_user_alternatives_after_reset = options_with_defaults[:retain_user_alternatives_after_reset] if self.algorithm == Split::Algorithms::SystematicSampling - self.cohorting_block_seed = options_with_defaults[:cohorting_block_seed] || self.name.to_i(36) + self.cohorting_block_seed = options_with_defaults[:cohorting_block_seed] || self.name.sum self.cohorting_block_magnitude = options_with_defaults[:cohorting_block_magnitude] end end @@ -443,8 +443,8 @@ def enable_cohorting redis.hset(experiment_config_key, :cohorting, false) end - def next_cohorting_block_index - Split.redis.incr("#{name}:cohorting_block_index") - 1 + def next_cohorting_block_count + Split.redis.incr("#{name}:cohorting_block_count") - 1 end protected diff --git a/spec/algorithms/systematic_sampling_spec.rb b/spec/algorithms/systematic_sampling_spec.rb index a16d88d6..7cb3b9da 100644 --- a/spec/algorithms/systematic_sampling_spec.rb +++ b/spec/algorithms/systematic_sampling_spec.rb @@ -22,7 +22,6 @@ results[Split::Algorithms::SystematicSampling.choose_alternative(experiment).name] += 1 end - expect(experiment.cohorting_block_magnitude * experiment.alternatives.length).to eq(6) expect(results).to eq({'red' => 2, 'blue' => 2, 'green' => 2}) end @@ -36,7 +35,6 @@ results[Split::Algorithms::SystematicSampling.choose_alternative(experiment).name] += 1 end - expect(experiment.cohorting_block_magnitude * experiment.alternatives.length).to eq(6) expect(results).to eq({'red' => 2, 'blue' => 2, 'green' => 2}) end end @@ -52,7 +50,8 @@ end let(:seeded_experiment2) do - Split::Experiment.new('link_highlight', + Split::Experiment.new( + 'link_highlight', :alternatives => ['red', 'blue', 'green'], :algorithm => Split::Algorithms::SystematicSampling, :cohorting_block_seed => 1234) diff --git a/spec/experiment_spec.rb b/spec/experiment_spec.rb index db1dce91..763d5805 100644 --- a/spec/experiment_spec.rb +++ b/spec/experiment_spec.rb @@ -167,10 +167,10 @@ def alternative(color) it 'when the experiment is using SystematicSampling algorithm, assigns cohorting_block_seed to a default value' do expect(Split::Experiment.new('systematic_sampling_exp', algorithm: Split::Algorithms::SystematicSampling).cohorting_block_seed) - .to eq(387211932128858527590062598078109) + .to eq(2476) expect(Split::Experiment.new('systematic_sampling_exp2', algorithm: Split::Algorithms::SystematicSampling).cohorting_block_seed) - .to eq(13939629556638906993242253530811926) + .to eq(2526) end it "sets friendly_name" do @@ -467,20 +467,20 @@ def alternative(color) end end - describe '#next_cohorting_block_index' do + describe '#next_cohorting_block_count' do it 'increments each call and starts at 0' do - expect(experiment.next_cohorting_block_index).to eq(0) - expect(experiment.next_cohorting_block_index).to eq(1) - expect(experiment.next_cohorting_block_index).to eq(2) - expect(experiment.next_cohorting_block_index).to eq(3) + expect(experiment.next_cohorting_block_count).to eq(0) + expect(experiment.next_cohorting_block_count).to eq(1) + expect(experiment.next_cohorting_block_count).to eq(2) + expect(experiment.next_cohorting_block_count).to eq(3) end it 'persists value' do - expect(experiment.next_cohorting_block_index).to eq(0) + expect(experiment.next_cohorting_block_count).to eq(0) experiment.save e = Split::ExperimentCatalog.find("link_color") - expect(e.next_cohorting_block_index).to eq(1) + expect(e.next_cohorting_block_count).to eq(1) end end From 1730b9d3ac3abca324c7181a5a17c1884ae77b35 Mon Sep 17 00:00:00 2001 From: Rowan441 Date: Fri, 29 Apr 2022 13:33:31 -0400 Subject: [PATCH 4/4] seperate alorithm counts for different versions --- lib/split/experiment.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/split/experiment.rb b/lib/split/experiment.rb index 405ebdf3..501e28e9 100644 --- a/lib/split/experiment.rb +++ b/lib/split/experiment.rb @@ -444,7 +444,7 @@ def enable_cohorting end def next_cohorting_block_count - Split.redis.incr("#{name}:cohorting_block_count") - 1 + Split.redis.incr("#{key}:cohorting_block_count") - 1 end protected