Standard error margin #224

slobodan-ilic · 2020-10-28T13:04:35Z

Mostly add expectations, but also implement the feature.

src/cr/cube/cubepart.py

ernestoarbitrio

Left some comments ...

coveralls · 2020-10-28T15:26:56Z

Coverage remained the same at 100.0% when pulling 21d6d76 on standard-error-margin into 85cd116 on master.

ernestoarbitrio

Code LGTM to me :)

scanny

A few changes, mostly just updates on the docstring and test conventions we've evolved to while you were away :)

scanny · 2020-10-28T18:10:04Z

src/cr/cube/cubepart.py

+        """Returns the margin of error (MoE) for col percentages
+        `moe = Z_975 * 100 * std_error` (the * 100 part accounts for percentages)
+        """


Rename to column_percentages_moe so it is explicit and also appears immediately following column_percentages, both here in the code and in the documentation.

Prefer "float margin of error (MoE) for column percentages." for the summary line. A property, like an attribute, doesn't "return" anything, it "is" some value. Although there are many examples (like the next property below) left to clean up, we reserve the "Returns ... prefix for methods and functions.

Blank line before continuation when the docstring extends beyond the first line: https://www.python.org/dev/peps/pep-0257/#multi-line-docstrings

moe = Z_975 * 100 * std_error just restates the implementation. This docstring appears in the documentation and so should communicate to the user what they are getting from (the contract for) this property, in user-space terms. It should also probably state that 3.5% is represented as 3.5 (and not .035). Also it should probably mention that the return value can be NaN and under what circumstances. And we should state clearly whether a returned NaN is a float("NaN") or np.nan because those two have different behaviors in certain comparisons.

The (the * 100 ...) bit can appear as a comment and seems useful. Comments speak in developer-space and topics related to the implementation are appropriate there.

scanny · 2020-10-28T18:16:06Z

src/cr/cube/cubepart.py

@@ -239,6 +239,10 @@ class _Slice(CubePartition):
    dimensions which can be crosstabbed in a slice.
    """

+    # quantile of the normal cdf at .975 because the confidence


Don't make me look up what cdf is. Spell it out and then show the abbreviation in parentheses, like Common Denominator Frombus (CDF). Also, if you would, prefix comments with --- to make them stand out from commented-out code. Like:

# --- Quantile of the normal Cubic Dimension Fromulus (CDF) at .975 because the # --- confidence interval is ±.025 on each side.

What are the units of the ±.025 value? Is that standard-deviations? Best to be explicit. Use these opportunities to educate the uninitiated reader.

scanny · 2020-10-28T18:18:26Z

src/cr/cube/cubepart.py

-        """Returns the standard deviation for cell percentages
+        """Returns the standard deviation for col percentages


Good catch, remainder of fixup would be something like:

"""np.float64 standard-deviation of column percentages."""

scanny · 2020-10-28T18:19:12Z

src/cr/cube/cubepart.py

+              0     1     2     3      4       5       6
+        0   inf   inf   inf   inf    inf    -2.9     inf
+        1   inf   inf   inf   inf    inf    -4.3     inf
+        2   2.5   1.3   3.3  -0.70  -7.25   -6.52    2.25
+        3   inf   inf   inf   inf    inf    -2.51    inf
+        4  -1.16  2.20  5.84  1.78  -8.48   -5.92    0.93
+        5   inf   inf   inf   inf    inf     9.70    inf
+
+        Only the insertions residuals are showed in a inf masked array"""


👍 this scattered alignment with tabs in it has been bugging me for months :)

Final """ should appear on its own line. From the PEP257 link above:

Unless the entire docstring fits on a line, place the closing quotes on a line by themselves.

scanny · 2020-10-28T18:23:10Z

src/cr/cube/cubepart.py

-        """ -> np.int64 ndarray of the columns scale median
+        """-> np.int64 ndarray of the columns scale median


We tried out this -> shorthand for "Returns ..." because it was compact and consistent with Python3 type-hints. But the new version of Black won't allow the leading space and then the eye struggles to differentiate it from the """. So we're back to using Returns when applicable (methods and functions). This is a property though, so the arrow should just go away, leaving """np.int64 ndarray of ...""".

scanny · 2020-10-28T18:27:08Z

src/cr/cube/cubepart.py

+        """Returns the margin of error (MoE) for table percentages
+        `moe = Z_975 * 100 * std_error` (the * 100 part accounts for percentages)
+        """
+        return self.Z_975 * 100 * self.table_std_err


same remarks as on column_moe.

scanny · 2020-10-28T18:32:26Z

tests/integration/test_headers_and_subtotals.py

@@ -1127,6 +1127,65 @@ def it_calculate_col_residuals_for_subtotals(self):
                ],
            ],
        )
+        np.testing.assert_almost_equal(


this expectation should be extracted. It's long enough to disrupt reading the test.

scanny · 2020-10-28T18:34:31Z

tests/integration/test_multiple_response.py

@@ -375,11 +375,58 @@ def test_various_measures_from_r_rows_margin():
        ],
    ]

+    expected_col_moe = [


all of the expectations for this test should be extracted. Together they are like three screenfuls of noise in the test. We're doing this incrementally as we encounter legacy tests like this one.

scanny

A couple small remarks, mostly for future reference, but approved in advance.

scanny · 2020-10-30T17:02:19Z

src/cr/cube/cubepart.py

@@ -764,6 +780,17 @@ def table_margin(self):
    def table_margin_unpruned(self):
        return self._matrix.table_margin_unpruned

+    @lazyproperty
+    def table_percentages_moe(self):


out of alphabetical order.

scanny · 2020-10-30T17:07:56Z

src/cr/cube/cubepart.py

-        """Returns the variance for cell percentages
+        """Returns the variance for column percentages


Not asking you to change all these because they snuck in on prior commits, but the "Return" at the beginning should be imperative tense rather than passive tense. So like "Return the variance ..." like you were telling this property what to do rather than "Returns the variance ..." like it was a passive description. If you check the Python documentation you'll see they do it this way. Generally I make all these little fixups if I have occasion to make any change to a docstring.

slobodan-ilic added 3 commits October 27, 2020 16:58

black

105a55e

tests: Margin of Error for columns and table

c4c5f3b

feat: implement MoE

dc4327e