Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions lib/split.rb
Original file line number Diff line number Diff line change
@@ -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'
Expand Down
18 changes: 18 additions & 0 deletions lib/split/algorithms/systematic_sampling.rb
Original file line number Diff line number Diff line change
@@ -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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Q] Is there any concerns/considerations that need to be taken here with experiment.participant_count and concurrency/race conditions ~ when say 2 accounts are being cohorted at the same time and both retrieve the same participant_count.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, thats definitely a concern, the participant count is incremented in choose!() after this algorithm returns a value. I think the solution to this would be to store our own counter field in redis, then at the beginning of the algoritihm we can call incrby on that values and use the return value from it as the index

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
3 changes: 2 additions & 1 deletion lib/split/configuration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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|
Expand Down
16 changes: 16 additions & 0 deletions lib/split/experiment.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -17,6 +18,7 @@ class Experiment
DEFAULT_OPTIONS = {
:resettable => true,
:retain_user_alternatives_after_reset => false,
:cohorting_block => ["control", "alternative"]
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I put a default value of ["control", "alternative"] here, we can set this to whatever we want. The problem currently is that if someone is using alternatives other than 'control' and 'alternative' the default won't work and they will have to override it

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm not sure how I feel about storing specific metadata for an algorithm at the experiment level without a more generic way of handling/storing it.

}

def initialize(name, options = {})
Expand All @@ -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)
Expand All @@ -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

Expand Down Expand Up @@ -232,6 +236,10 @@ def friendly_name_key
"#{name}:friendly_name"
end

def cohorting_block_key
"#{name}:cohorting_block"
end

def resettable?
resettable
end
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
62 changes: 62 additions & 0 deletions spec/algorithms/systematic_sampling_spec.rb
Original file line number Diff line number Diff line change
@@ -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
15 changes: 15 additions & 0 deletions spec/experiment_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down