From 98abc7bb894d7724dd0ca448ee4c2fcef858f8b5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9bastien=20Fauvel?= Date: Tue, 11 Nov 2014 12:27:02 +0100 Subject: [PATCH 1/3] Allowed for zero-weight, negative-weight and multiple split tests conversions - Added migration script to start the conversion weights at 0 - Now adding up successive conversion weights instead of overwriting them - Allowed for zero-weight and negative-weight conversions, adjusted significance calculation accordingly - Adjusted tests --- library/CM/Model/Splittest.php | 15 +++++++-------- library/CM/Model/SplittestVariation.php | 10 +++++++--- resources/db/structure.sql | 2 +- resources/db/update/39.php | 7 +++++++ tests/library/CM/Model/Splittest/UserTest.php | 1 + tests/library/CM/Model/SplittestVariationTest.php | 13 ++++++------- 6 files changed, 29 insertions(+), 19 deletions(-) create mode 100644 resources/db/update/39.php diff --git a/library/CM/Model/Splittest.php b/library/CM/Model/Splittest.php index 39fe77361..9c745ca2c 100644 --- a/library/CM/Model/Splittest.php +++ b/library/CM/Model/Splittest.php @@ -180,16 +180,14 @@ protected function _setConversion(CM_Splittest_Fixture $fixture, $weight = null) if (null === $weight) { $weight = 1; } - if ($weight <= 0) { - throw new CM_Exception_Invalid('Weight must be positive or null, `' . $weight . '` given'); - } $weight = (float) $weight; $fixtureId = $fixture->getId(); - $columnId = $fixture->getColumnId(); + $columnIdQuoted = CM_Db_Db::getClient()->quoteIdentifier($fixture->getColumnId()); - CM_Db_Db::update('cm_splittestVariation_fixture', - array('conversionStamp' => time(), 'conversionWeight' => $weight), - array('splittestId' => $this->getId(), $columnId => $fixtureId)); + CM_Db_Db::exec('UPDATE `cm_splittestVariation_fixture` + SET `conversionStamp` = COALESCE(`conversionStamp`, ?), + `conversionWeight` = `conversionWeight` + ? + WHERE `splittestId` = ? AND ' . $columnIdQuoted . ' = ?', array(time(), $weight, $this->getId(), $fixtureId)); } /** @@ -208,6 +206,7 @@ protected function _isVariationFixture(CM_Splittest_Fixture $fixture, $variation */ protected function _getVariationFixture(CM_Splittest_Fixture $fixture) { $columnId = $fixture->getColumnId(); + $columnIdQuoted = CM_Db_Db::getClient()->quoteIdentifier($columnId); $fixtureId = $fixture->getId(); $cacheKey = CM_CacheConst::Splittest_VariationFixtures . '_id:' . $fixture->getId() . '_type:' . $fixture->getFixtureType(); @@ -218,7 +217,7 @@ protected function _getVariationFixture(CM_Splittest_Fixture $fixture) { SELECT `variation`.`splittestId`, `variation`.`name` FROM `cm_splittestVariation_fixture` `fixture` JOIN `cm_splittestVariation` `variation` ON(`variation`.`id` = `fixture`.`variationId`) - WHERE `fixture`.`' . $columnId . '` = ?', array($fixtureId))->fetchAllTree(); + WHERE `fixture`.' . $columnIdQuoted . ' = ?', array($fixtureId))->fetchAllTree(); $cacheWrite = true; } diff --git a/library/CM/Model/SplittestVariation.php b/library/CM/Model/SplittestVariation.php index c8bb9b949..70cdfd1fa 100644 --- a/library/CM/Model/SplittestVariation.php +++ b/library/CM/Model/SplittestVariation.php @@ -101,12 +101,16 @@ public function getSignificance(CM_Model_SplittestVariation $variationWorse) { $conversionsExpectedA = $rateTotal * $fixturesA / $netRateA; $conversionsExpectedB = $rateTotal * $fixturesB / $netRateB; - $sigmaExpectedA = sqrt($conversionsExpectedA * (1 - $conversionsExpectedA / $fixturesA)); - $sigmaExpectedB = sqrt($conversionsExpectedB * (1 - $conversionsExpectedB / $fixturesB)); + $varianceExpectedA = $conversionsExpectedA * (1 - $conversionsExpectedA / $fixturesA); + $varianceExpectedB = $conversionsExpectedB * (1 - $conversionsExpectedB / $fixturesB); - if ($sigmaExpectedA < 3 || $sigmaExpectedB < 3) { + if ($varianceExpectedA < 9 || $varianceExpectedB < 9) { return null; } + + $sigmaExpectedA = sqrt($varianceExpectedA); + $sigmaExpectedB = sqrt($varianceExpectedB); + if ($conversionsExpectedA - 3 * $sigmaExpectedA < 0 || $conversionsExpectedB - 3 * $sigmaExpectedB < 0) { return null; } diff --git a/resources/db/structure.sql b/resources/db/structure.sql index 3ace4a591..8480fbda9 100644 --- a/resources/db/structure.sql +++ b/resources/db/structure.sql @@ -317,7 +317,7 @@ CREATE TABLE `cm_splittestVariation_fixture` ( `variationId` int(10) unsigned NOT NULL, `createStamp` int(10) unsigned NOT NULL, `conversionStamp` int(11) DEFAULT NULL, - `conversionWeight` decimal(10,2) NOT NULL DEFAULT '1.00', + `conversionWeight` decimal(10,2) NOT NULL DEFAULT '0.00', UNIQUE KEY `userSplittest` (`userId`,`splittestId`), UNIQUE KEY `requestClientSplittest` (`requestClientId`,`splittestId`), KEY `splittestId` (`splittestId`), diff --git a/resources/db/update/39.php b/resources/db/update/39.php new file mode 100644 index 000000000..f1c35b918 --- /dev/null +++ b/resources/db/update/39.php @@ -0,0 +1,7 @@ +getDefaultValue()) { + CM_Db_Db::exec('ALTER TABLE cm_splittestVariation_fixture + MODIFY COLUMN conversionWeight decimal(10,2) NOT NULL DEFAULT 0'); + CM_Db_Db::update('cm_splittestVariation_fixture', array('conversionWeight' => 0), array('conversionStamp' => null)); +} diff --git a/tests/library/CM/Model/Splittest/UserTest.php b/tests/library/CM/Model/Splittest/UserTest.php index abd5fcf42..d9cf82bb2 100644 --- a/tests/library/CM/Model/Splittest/UserTest.php +++ b/tests/library/CM/Model/Splittest/UserTest.php @@ -59,6 +59,7 @@ public function testSetConversionStatic() { CM_Model_Splittest_User::setConversionStatic('foo', $user1); $this->assertSame(1, $variation->getConversionCount(true)); CM_Model_Splittest_User::setConversionStatic('foo', $user2, 2.5); + $this->assertSame(2, $variation->getConversionCount(true)); $this->assertSame(1.75, $variation->getConversionRate(true)); } } diff --git a/tests/library/CM/Model/SplittestVariationTest.php b/tests/library/CM/Model/SplittestVariationTest.php index 6640d53a3..d0124ffce 100644 --- a/tests/library/CM/Model/SplittestVariationTest.php +++ b/tests/library/CM/Model/SplittestVariationTest.php @@ -96,18 +96,17 @@ public function testGetConversionWeight() { $test->isVariationFixture($user, 'v1'); $test->isVariationFixture($user2, 'v1'); $test->isVariationFixture($user3, 'v1'); + $this->assertSame(0, $variation->getConversionCount(true)); $this->assertSame(0.0, $variation->getConversionWeight(true)); $test->setConversion($user, 3.75); $test->setConversion($user2, 3.29); + $this->assertSame(2, $variation->getConversionCount(true)); $this->assertSame(7.04, $variation->getConversionWeight(true)); $this->assertSame(2.3466666666667, $variation->getConversionRate(true)); - - try { - $test->setConversion($user, -2); - $this->fail('Could set Conversion with negative weight'); - } catch (CM_Exception_Invalid $e) { - $this->assertTrue(true); - } + $test->setConversion($user, -2); + $this->assertSame(2, $variation->getConversionCount(true)); + $this->assertSame(5.04, $variation->getConversionWeight(true)); + $this->assertSame(1.68, $variation->getConversionRate(true)); $test->delete(); } From 91b43b0a72a08aed1f9828b725c6913f8fff7848 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9bastien=20Fauvel?= Date: Tue, 11 Nov 2014 12:30:09 +0100 Subject: [PATCH 2/3] Formatting --- library/CM/Model/Splittest.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/library/CM/Model/Splittest.php b/library/CM/Model/Splittest.php index 9c745ca2c..bb98e9780 100644 --- a/library/CM/Model/Splittest.php +++ b/library/CM/Model/Splittest.php @@ -187,7 +187,8 @@ protected function _setConversion(CM_Splittest_Fixture $fixture, $weight = null) CM_Db_Db::exec('UPDATE `cm_splittestVariation_fixture` SET `conversionStamp` = COALESCE(`conversionStamp`, ?), `conversionWeight` = `conversionWeight` + ? - WHERE `splittestId` = ? AND ' . $columnIdQuoted . ' = ?', array(time(), $weight, $this->getId(), $fixtureId)); + WHERE `splittestId` = ? AND ' . $columnIdQuoted . ' = ?', + array(time(), $weight, $this->getId(), $fixtureId)); } /** From fedaa093456ef8f4feb7139f32ffc68cdd39809f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9bastien=20Fauvel?= Date: Tue, 11 Nov 2014 12:53:38 +0100 Subject: [PATCH 3/3] Only allowed for multiple conversions if a weight is explicitly given - Added tests --- library/CM/Model/Splittest.php | 23 ++++++---- .../CM/Model/SplittestVariationTest.php | 46 +++++++++++++++++++ 2 files changed, 59 insertions(+), 10 deletions(-) diff --git a/library/CM/Model/Splittest.php b/library/CM/Model/Splittest.php index bb98e9780..f535ab11f 100644 --- a/library/CM/Model/Splittest.php +++ b/library/CM/Model/Splittest.php @@ -177,18 +177,21 @@ protected function _getContainingCacheables() { * @throws CM_Exception_Invalid */ protected function _setConversion(CM_Splittest_Fixture $fixture, $weight = null) { + $columnId = $fixture->getColumnId(); + $fixtureId = $fixture->getId(); if (null === $weight) { - $weight = 1; + CM_Db_Db::update('cm_splittestVariation_fixture', + array('conversionStamp' => time(), 'conversionWeight' => 1), + array('splittestId' => $this->getId(), $columnId => $fixtureId, 'conversionStamp' => null)); + } else { + $weight = (float) $weight; + $columnIdQuoted = CM_Db_Db::getClient()->quoteIdentifier($columnId); + CM_Db_Db::exec('UPDATE `cm_splittestVariation_fixture` + SET `conversionStamp` = COALESCE(`conversionStamp`, ?), + `conversionWeight` = `conversionWeight` + ? + WHERE `splittestId` = ? AND ' . $columnIdQuoted . ' = ?', + array(time(), $weight, $this->getId(), $fixtureId)); } - $weight = (float) $weight; - $fixtureId = $fixture->getId(); - $columnIdQuoted = CM_Db_Db::getClient()->quoteIdentifier($fixture->getColumnId()); - - CM_Db_Db::exec('UPDATE `cm_splittestVariation_fixture` - SET `conversionStamp` = COALESCE(`conversionStamp`, ?), - `conversionWeight` = `conversionWeight` + ? - WHERE `splittestId` = ? AND ' . $columnIdQuoted . ' = ?', - array(time(), $weight, $this->getId(), $fixtureId)); } /** diff --git a/tests/library/CM/Model/SplittestVariationTest.php b/tests/library/CM/Model/SplittestVariationTest.php index d0124ffce..aeb29eb38 100644 --- a/tests/library/CM/Model/SplittestVariationTest.php +++ b/tests/library/CM/Model/SplittestVariationTest.php @@ -111,6 +111,52 @@ public function testGetConversionWeight() { $test->delete(); } + public function testGetConversionWeight_SingleConversion() { + $user = CMTest_TH::createUser(); + + /** @var CM_Model_Splittest_User $test */ + $test = CM_Model_Splittest_User::createStatic(array('name' => 'bar', 'variations' => array('v1'))); + /** @var CM_Model_SplittestVariation $variation */ + $variation = $test->getVariations()->getItem(0); + + $test->isVariationFixture($user, 'v1'); + $this->assertSame(0, $variation->getConversionCount(true)); + $this->assertSame(0.0, $variation->getConversionWeight(true)); + $test->setConversion($user); + $this->assertSame(1, $variation->getConversionCount(true)); + $this->assertSame(1., $variation->getConversionWeight(true)); + $this->assertSame(1., $variation->getConversionRate(true)); + $test->setConversion($user); + $this->assertSame(1, $variation->getConversionCount(true)); + $this->assertSame(1., $variation->getConversionWeight(true)); + $this->assertSame(1., $variation->getConversionRate(true)); + + $test->delete(); + } + + public function testGetConversionWeight_MultipleConversions() { + $user = CMTest_TH::createUser(); + + /** @var CM_Model_Splittest_User $test */ + $test = CM_Model_Splittest_User::createStatic(array('name' => 'bar', 'variations' => array('v1'))); + /** @var CM_Model_SplittestVariation $variation */ + $variation = $test->getVariations()->getItem(0); + + $test->isVariationFixture($user, 'v1'); + $this->assertSame(0, $variation->getConversionCount(true)); + $this->assertSame(0.0, $variation->getConversionWeight(true)); + $test->setConversion($user, 1.); + $this->assertSame(1, $variation->getConversionCount(true)); + $this->assertSame(1., $variation->getConversionWeight(true)); + $this->assertSame(1., $variation->getConversionRate(true)); + $test->setConversion($user, 1.); + $this->assertSame(1, $variation->getConversionCount(true)); + $this->assertSame(2., $variation->getConversionWeight(true)); + $this->assertSame(2., $variation->getConversionRate(true)); + + $test->delete(); + } + public function testGetFixtureCount() { $user1 = CMTest_TH::createUser(); $user2 = CMTest_TH::createUser();