feat: Refactor of the power calculator store #43

threkk · 2021-06-23T08:51:15Z

TODO

The new source of truth is the store and the refactor avoids the data duplicity.

…rty and some sample cases.

… at non-inferiority

…visitors per day calculation for non-inferiorty.

…s, feature parity for non-inferiority.

…tate.

Vue.js does not get along with contenteditable. https://blog.francium.tech/vue-js-and-html5-contenteditable-attribute-issues-c727734c7684

The integration was having some issues when importing props. With this patch: - It will use only the actual format for the prop import instead of supporting the old format. It is up to the integration layer to adapt it. - Once the values are imported, it will automatically refresh possible missing values and emit an event with the new values.

…rong standard deviation

arrudaje

Hey, most of my comments are just CTRL+C CTRL+V, so don't panic 😅

arrudaje · 2021-06-23T13:59:59Z

src/components/base-comp.vue

+  >
+    <pc-svg-chain :isBlockFocused="isBlockFocused"></pc-svg-chain>
+
+    <div class="pc-header" v-if="testType == 'gTest'">Base Rate</div>


Why not === ?

Mostly this is code that already was like that and I didn't really touch, just formatting. However, it is a good point, and will get fixed.

arrudaje · 2021-06-23T14:00:49Z

src/components/base-comp.vue

+      <li class="pc-input-item pc-input-left">
+        <label>
+          <span class="pc-input-title"
+            >{{ testType == 'gTest' ? 'Base Rate' : 'Base Average' }}


Why not === ? Also, if we expect this condition to repeat, shouldn't we wrap it in a computed property?

arrudaje · 2021-06-23T14:01:28Z

src/components/base-comp.vue

+        </label>
+      </li>
+
+      <li class="pc-input-item pc-input-sd-rate" v-if="testType == 'tTest'">


arrudaje · 2021-06-23T14:39:56Z

src/components/base-comp.vue

+            :fieldValue.sync="sdRate"
+            :isReadOnly="isReadOnly"
+            :isBlockFocused="isBlockFocused"
+            enableEdit="true"


Please don't use the raw property, this assigns the prop as string, and not boolean as it's the original intention. If we decide to modify this to false it will not have the intended effect. Just add the : to the beginning of the prop name.

arrudaje · 2021-06-23T14:47:47Z

src/components/base-comp.vue

+        return this.$store.getters.baseRate
+      },
+      set(val) {
+        if (this.baseDebouncer != null) {


Here you also should make the comparison strict, as the baseDebouncer is defined initially with null so it will not become undefined

The normal uneven comparison is a trick: it checks that the value is not null or undefined. Strict check will return true for undefined.

arrudaje · 2021-06-23T15:33:53Z

src/components/sample-comp.vue

+      }
+    },
+    getLockedStateClass(param) {
+      return this.lockedField == param


arrudaje · 2021-06-23T15:39:11Z

src/powercalculator.vue

+              fieldProp="variants"
+              prefix="base + "
+              :fieldValue.sync="variants"
+              enableEdit="true"


Raw property~

arrudaje · 2021-06-23T15:39:18Z

src/powercalculator.vue

+              suffix="%"
+              fieldProp="falsePositiveRate"
+              :fieldValue.sync="falsePositiveRate"
+              enableEdit="true"


Raw property~

arrudaje · 2021-06-23T15:39:27Z

src/powercalculator.vue

+              :class="{ 'pc-top-fields-error': power < 80 }"
+              fieldProp="power"
+              :fieldValue.sync="power"
+              enableEdit="true"


Raw property~

arrudaje · 2021-06-23T15:39:38Z

src/powercalculator.vue

+
+      <div
+        class="pc-blocks-wrapper"
+        :class="{ 'pc-blocks-wrapper-ttest': testType == 'tTest' }"


arrudaje

LGTM

…the adjusted value When the distribution is binomial, the scale of the base rate is adjusted from normal value to percentage value to match the display of the value. However, the value used to calculate the missing value of the relative/absolute pair was using the original one instead of the adjusted.

…odes During the last refactoring of the power calculator, one of the rules we imposed ourselves was to respect user input. This is, if the user wrote "400", after switching it would still be "400", but changing the equivalent internally from "400 units" to "400%". The best solution is to remove the % symbol in the binomial metrics. That way, we don't need to transform the internal values for the user input to stay the same, and will avoid user confusion.

…ue instead of the new one

…the base rate

…nt with the base rate" This reverts commit 29d5892.

…etween modes" This reverts commit 46b8ea1.

This reverts part of the commit 374ceb8, but keeps the original purpose.

…nferiority tests and its effect on the false positive rate

threkk added 30 commits February 11, 2021 14:25

feat: Enables impact calculation for non-inferiority tests

bd483bd

fix: Possible solution for the calculation of non-inferiority.

43313e6

feat: power calculator store prototype

22330d4

wip: Refactors the pc-block-value to separate data from UI.

7093b9b

The new source of truth is the store and the refactor avoids the data duplicity.

wip: Fixes non-inferiority component.

a8c8c31

wip: Gets the base component working and the others to display.

8e2e321

wip: Sample size working for changes in the block when calculating

166918a

wip: Calculate base and impact work for binomial. Pending non-inferio…

9f7ea69

…rty and some sample cases.

wip: Everything working for comparative binomial.

321f4a2

wip: Continuos distribution working.

001c73d

wip: non-inferiority displays default data

c69b0a6

wip: Feature parity on not non-inferiority. First calculation attempt…

15e0ffa

… at non-inferiority

gip: Gets the right configuration for blocking total fields. Enables …

4cb0891

…visitors per day calculation for non-inferiorty.

wip: Simplifies mutations (1 function per mutation), fixes math error…

877386e

…s, feature parity for non-inferiority.

wip: Added the calculation of threshold for non-inferiority.

8494e06

wip: Removes the bottom graph.

2645d53

wip: Methods for hooking the new power calculator to the old app.

8a474ac

fix: Input validation and visual glitches.

2074424

fix: Power calculator props, event issues.

d3926ac

wip: Modifies the underlying engine to match each user input with a s…

0fcd107

…tate.

wip: Fixes after intense debugging sessions.

b46b37e

wip: Fixes stddev hooks and the threshold bouncer

730eb86

wip: threshold calculation fixes.

94359ea

fix: usage of the right threshold value.

45aacf4

wip: Fixes import, removes expected change.

5d5ee6b

wip: Fixes import errors.

539afd1

fix: All reported visual bugs.

60daad3

fix: Changes {{ }} for v-html

f304a10

Vue.js does not get along with contenteditable. https://blog.francium.tech/vue-js-and-html5-contenteditable-attribute-issues-c727734c7684

wip: Adds absolute impact as a parameter.

92f582f

threkk added 8 commits May 21, 2021 15:55

fix: absoluteThreshold was being calculated using the previous base rate

755c3cf

test: Add gTest comparative tests

6c38790

test: Adds a lot of tests.

cacf700

feat: Using tTest formulas for binomial non inferiority distributions.

e48d3b8

test: Clean up of invalid tests.

6f7d9e9

chore: Update bundle

d3adc5b

fix: changes with fixed days was breaking due to wrong property name

1a82fa8

fix: late assignment of new test type and base rate was picking the w…

6754eff

…rong standard deviation

arrudaje suggested changes Jun 23, 2021

View reviewed changes

threkk added 2 commits June 24, 2021 12:25

fix: Addresses pull request code style recommendations.

1618c1e

chore: bundle update

92cb06d

arrudaje approved these changes Jun 24, 2021

View reviewed changes

threkk added 18 commits June 25, 2021 11:30

fix: Fixes wrong property path.

4e0078c

fix: Removes the s from 'days' when runtime is 1

180d0b6

feat: Sets tabindex for power users.

bf945a9

feat: UI will not round up user input for display purposes.

1035da3

fix: the mix/maxAbsoluteImpact was wrongly formatted.

1b6a6a6

fix: display of absolute impact for binomial calculations

546dc4a

fix: the absolute threshold was calculated using the old visitors val…

ad93b64

…ue instead of the new one

fix: changes the display of the absolute value to be consistent with …

29d5892

…the base rate

fix: removes the trailing dot when it is the last character

374ceb8

test wip

6addd7f

Revert "fix: changes the display of the absolute value to be consiste…

2b45f7c

…nt with the base rate" This reverts commit 29d5892.

Revert "feat: changes the display of the base rate to be consistent b…

041d412

…etween modes" This reverts commit 46b8ea1.

fix: removes the trailing dot when it is the last character

dc9766b

This reverts part of the commit 374ceb8, but keeps the original purpose.

test: fixes some unit tests

bda09f7

test: fixes the gTest-sample

19d177a

feat: makes explicit the internal calculation that happens with non-i…

45b1249

…nferiority tests and its effect on the false positive rate

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Refactor of the power calculator store #43

feat: Refactor of the power calculator store #43

threkk commented Jun 23, 2021

arrudaje left a comment

arrudaje Jun 23, 2021

threkk Jun 24, 2021

arrudaje Jun 23, 2021

threkk Jun 24, 2021

arrudaje Jun 23, 2021

threkk Jun 24, 2021

arrudaje Jun 23, 2021

arrudaje Jun 23, 2021

threkk Jun 24, 2021

arrudaje Jun 23, 2021

arrudaje Jun 23, 2021

arrudaje Jun 23, 2021

arrudaje Jun 23, 2021

arrudaje Jun 23, 2021

arrudaje left a comment

feat: Refactor of the power calculator store #43

Are you sure you want to change the base?

feat: Refactor of the power calculator store #43

Conversation

threkk commented Jun 23, 2021

arrudaje left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arrudaje left a comment

Choose a reason for hiding this comment