Skip to content
This repository has been archived by the owner on Jul 29, 2024. It is now read-only.

Refactored randomization module #24 #30

Merged
merged 1 commit into from
Mar 30, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
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
107 changes: 107 additions & 0 deletions src/randomization.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
/**
* This module enables running measurements and interventions with randomization,
* such as A/B tests, multivariate tests, and randomized controlled trials.
*
* @module webScience.randomization
*/

/**
* A condition for a measurement or intervention that can be randomly selected.
* @typedef {Object} Condition
* @property {string} name - A name that uniquely identifies the condition within
* the set of conditions.
* @property {number} weight - The positive weight to give this condition when randomly
* selecting a condition from a set.
*/

/**
* @typedef {Object} ConditionSet
* @property {string} name - A name that uniquely identifies the set of conditions.
* @property {Condition[]} conditions - The conditions in the set.
*/

/**
* @type {Object|null}
* @private
* A map of condition set names to condition names. Maintaining a cache avoids
* storage race conditions. The cache is an Object rather than a Map so it can
* be easily stored in extension local storage.
*/
let conditionCache = null;

/**
* @type {string}
* @const
* @private
* A unique key for storing selected conditions in extension local storage.
*/
const storageKey = "webScience.randomization.conditions";

/**
* Selects a condition from a set of conditions. If a condition has previously
* been selected from the set, that same condition will be returned. If not,
* a condition will be randomly selected according to the provided weights.
* @param {ConditionSet} conditionSet - The set of conditions.
* @returns {string} - The name of the selected condition in the condition set.
* @example
* // on first run, returns "red" with 0.5 probability and "blue" with 0.5 probability
* // on subsequent runs, returns the same value as before
* randomization.selectCondition({
* name: "color",
* conditions: [
* {
* name: "red",
* weight: 1
* },
* {
* name: "blue",
* weight: 1
* }
* ]
* });
*/
export async function selectCondition(conditionSet) {
// Initialize the cache of selected conditions
if(conditionCache === null) {
const retrievedConditions = await browser.storage.local.get(storageKey);
// Check the cache once more, to avoid a race condition
if(conditionCache === null) {
if(storageKey in retrievedConditions)
conditionCache = retrievedConditions[storageKey];
else
conditionCache = { };
}
}

// Try to load the selected condition from the cache
if(conditionSet.name in conditionCache)
return conditionCache[conditionSet.name];
Copy link
Contributor

Choose a reason for hiding this comment

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

I think always using braces is a better practice (someone always comes along and adds a condition without realizing) but let's do that with eslint and eslint --fix everything vs. bothering to do it here. I'll file an issue

Copy link
Contributor

Choose a reason for hiding this comment

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

(I mean it's not that terrible here because it's return or throwing below, but if someone comes along and inserts a new line before the return things fail in new and interesting ways)


// If there isn't a previously selected condition, select a condition,
// save it to the cache and extension local storage, and return it
let totalWeight = 0;
const conditionNames = new Set();
if(!Array.isArray(conditionSet.conditions) || conditionSet.length === 0)
throw "The condition set must include an array with at least one condition."
for(const condition of conditionSet.conditions) {
if(condition.weight <= 0)
throw "Condition weights must be positive values."
totalWeight += condition.weight;
if(conditionNames.has(condition.name))
throw "Conditions must have unique names."
conditionNames.add(condition.name);
}
let randomValue = Math.random();
Copy link
Contributor

Choose a reason for hiding this comment

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

I went down the rabbit-hole of how Math.random works nowadays, I guess it's using https://en.wikipedia.org/wiki/Xoroshiro128%2B now, which sounds better than I remember!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about using the Web Crypto API for better randomness. I think we're OK with Math.random in this context, since the goal is just to divide users into a set of cohorts with target relative populations. Those cohorts will change (e.g., users withdrawing), and if researchers run statistical tests they'll have to account for cohort size.

let selectedCondition = "";
for(const condition of conditionSet.conditions) {
randomValue -= (condition.weight / totalWeight);
if(randomValue <= 0) {
selectedCondition = condition.name;
break;
}
}
conditionCache[conditionSet.name] = selectedCondition;
// No need to wait for storage to complete
browser.storage.local.set({ [storageKey]: conditionCache });
return selectedCondition.repeat(1);
}
3 changes: 3 additions & 0 deletions src/webScience.js
Original file line number Diff line number Diff line change
Expand Up @@ -53,3 +53,6 @@ export { linkExposure }

import * as socialMediaLinkSharing from "./socialMediaLinkSharing.js"
export { socialMediaLinkSharing }

import * as randomization from "./randomization.js"
export { randomization }
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we could do these as one line like: export { randomization } from './randomization.js? I haven't tried it yet but remember seeing it on MDN.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I looked into this. There's a pending proposal for namespace re-exports: https://github.com/tc39/proposal-export-ns-from

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, wasn't that merged in tc39/ecma262#1174 ? Although looking at the compatibility table on MDN we should probably avoid it for now anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I misunderstood the status of the proposal. Since we already need to use recent browser versions because of various WebExtensions APIs, I think we could switch to this syntax.