fix: null handling and numeric correctness in statistics, metrics and parsing by skymanbp · Pull Request #214 · DataHaskell/dataframe

skymanbp · 2026-08-19T13:32:24Z

The stat entry points read the raw payload of nullable columns, so the
sentinel stored at null slots entered every result: mean of
[10, null, 20] was 10.0, and on boxed columns sum/frequencies threw the
internal fromMaybeVec error at the user.

mean/median/percentile/genericPercentile/stddev/skewness/variance/IQR/sum
drop null slots through dropNulls :: Column -> Column; correlation does
listwise deletion; valueCounts (and so frequencies) no longer counts the
sentinel as a category. Maybe-typed views are untouched: their Nothings
are real values.
variance of n < 2 returned 0, so a singleton group looked exactly like a
constant one; now NaN, kernels included (throw vs NaN open in review).
skewness used a sqrt(n-1) factor: neither population g1 (the form the
docs define) nor the sample form. Now g1, matching scipy.stats.skew.
mse/mae/r2/accuracy/logLoss divided by length truth while zipWith
truncated to the shorter vector, and scored zero predictions as perfect;
they now average over compared pairs and throw on none.

The Int mean/median, correlation and double-parsing changes that were here
before are reverted per review and filed as #216/#217/#218 to come back as
separate PRs with benchmarks.

Full suite green locally.

Before / after

Before:

ghci> df = D.fromNamedColumns [("x", D.fromList [Just 1.0, Nothing, Just 3.0, Just 5.0]),
                               ("y", D.fromList [Just 2.0, Just 4.0, Just 6.0, Just 10.0])]
ghci> D.mean (F.col @Double "x") df
2.25                       -- null slot read as 0
ghci> D.correlation "x" "y" df
Just 0.9022436386781062    -- null pair included
ghci> mse VU.empty (VU.fromList [5,5,5])
0.0                        -- empty input scores "perfect"
ghci> D.skewness (F.col @Double "y") sk   -- sk = column [1,2,2,3,3,3,4,10]
1.6801920329205566

After:

ghci> D.mean (F.col @Double "x") df
3.0
ghci> D.correlation "x" "y" df
Just 1.0                   -- null pair dropped; remaining points are exactly y = 2x
ghci> mse VU.empty (VU.fromList [5,5,5])
*** Exception: [ERROR] mse cannot be called on empty data sets
ghci> D.skewness (F.col @Double "y") sk
1.796200837478836          -- scipy.stats.skew agrees

Stats read the raw payload of nullable columns, so the sentinel stored at null slots entered every result (a zero for unboxed columns, an error thunk for boxed ones): - mean/median/percentile/genericPercentile/stddev/skewness/variance/ IQR/sum now drop null slots via a shared dropNulls view. - correlation does listwise deletion over both columns. - valueCounts/valueProportions (and so frequencies) no longer count the sentinel as a category. - Maybe-typed views are untouched: their nulls are real values.

- Int mean/median widen per element instead of wrapping at 2^63. - correlation uses the centered two-pass form, clamped: the one-pass form returned |r| > 1, NaN, or a flipped sign on offset data. - skewness computes population g1, the formula the docs define; the old factor was off by sqrt((n-1)/n). - variance of n < 2 is NaN (scatter kernels included), not a fake 0 that made singleton groups look constant. - mse/mae/r2/accuracy/logLoss average over compared pairs and refuse to score zero predictions; meanSquaredError guards length mismatch. - readInt rejects overflow instead of wrapping. - Double parsing is correctly rounded: exact byte-level reference (clamped exponents), Clinger fast path, Infinity round-trips.

mchav · 2026-08-19T14:57:59Z

-                let var = if c < 2 then 0 else mm / fromIntegral (c - 1)
+                -- Sample variance is undefined at n = 1: NaN, matching
+                -- 'computeVariance'.
+                let var = if c < 2 then 0 / 0 else mm / fromIntegral (c - 1)


Hmmm. @daikonradish does it make sense to stick a NaN here or fail instead. I think the right thing is to make it optional but the ergonomics of dealing with Maybe make me question putting it in the happy path.

It's the same code the groupBy interpreter path uses... so Ig throwing there means one singleton group kills the whole aggregation (and the kernels would have to match)? NaN keeps the singleton visible without killing the query, and matches pandas/numpy (sample variance of n=1 -> NaN). Can simply switch all three to throw... (if u want it to fail loud)

mchav · 2026-08-19T15:08:39Z

+    App m _ | Just HRefl <- eqTypeRep m (typeRep @Maybe) -> xs
+    _ -> VG.backpermute xs keep
+  where
+    keep = VG.fromList [i | i <- [0 .. VG.length xs - 1], bitmapTestBit bm i]


keep = VG.filter (bitmapTestBit bm) (VG.enumFromN 0 (VG.length xs))

Will fuse and probably avoid the intermediate allocation. Since fromList doesn't know the final number of elements it grows its buffer dynamically which also could have a perf cost.

Went one further. DropNulls takes the column now and ifilters the payload directly, so there's no index vector at all.

mchav · 2026-08-19T15:10:33Z

 columnBitmap (PackedText bm _) = bm
 columnBitmap (MergedColumn _ _) = Nothing

+{- | Drop the null slots of a payload-typed view of a nullable column: those


-- | Drops the null values in a nullable column.

mchav · 2026-08-19T15:11:50Z

 mse preds truth
    | VU.null truth = 0
+    -- No predictions is not a perfect score.
+    | n == 0 = 0 / 0


Same as above. Nothing or throwing would make sense here.

Throws now (EmptyDataSetException). Also dropped the old empty-truth = 0 guard. unless if you want empty-truth = 0 back.

mchav · 2026-08-19T15:14:38Z

 mae preds truth
    | VU.null truth = 0
-    | otherwise = VU.sum (VU.zipWith (\p t -> abs (p - t)) preds truth) / n2 truth
+    | n == 0 = 0 / 0


Same here. Let's just throw.

mchav · 2026-08-19T15:21:49Z

-     in
-        Just $ squareDiff / fromIntegral (max (VU.length target) (VU.length prediction))
+meanSquaredError target prediction
+    | VU.length target /= VU.length prediction = Nothing


mchav · 2026-08-19T15:25:38Z

+    (Columnable a) => Expr a -> DataFrame -> Either DataFrameException (V.Vector a)
+columnAsVectorNonNull expr df = case expr of
+    Col name -> case getColumn name df of
+        Just col -> withColumnName name (dropNulls (columnBitmap col) <$> toVector col)


Now seeing how it's used this dropNulls function relies on getting the correspondence between the bitmap and the contents right even after they have been split apart. It should rather be dropNulls :: Column -> Column and the call site should be:

withColumnName name (toVector (dropNulls col))

Cool. DropNulls is Column -> Column. Tho one wrinkle is that a Maybe-typed view has to keep its Nothings, so the type-generic entry points (genericPercentile, valueCounts) go through a small dropNullsExceptMaybe guard; the numeric ones use dropNulls directly.

mchav · 2026-08-19T15:28:38Z

+columnAsVectorNonNull ::
+    forall a.
+    (Columnable a) => Expr a -> DataFrame -> Either DataFrameException (V.Vector a)
+columnAsVectorNonNull expr df = case expr of


It's not obvious from this function name that it does a non trivial operation (filter). All the other functions of this form do a cast at best and are clear about how they cast. Someone could run the function on two columns and get two different lengths then be shocked when zipWith doesn't work or maybe some stats functions don't throw. I don't have many good ideas here but let's rename this and I'm not sure if should be in the public API.

Renamed to nonNullElements. It was already private (not in the export list). Not sure tho if you'd rather a different name...

mchav · 2026-08-19T15:30:14Z

-w2d w
-    | w <= 9007199254740991 = fromIntegral (fromIntegral w :: Int)
-    | otherwise = fromInteger (toInteger w)
+w2d w = fromIntegral (fromIntegral w :: Int)


I'm not sure why this is necessary. Since these parsing functions are in the fast path they also deserver their own issue + PR. Haskell is really perf sensetive and I want to ensure we understand changes like this and their tradeoffs.

Fair. Just pulled out all the parsing changes. Thought both readers were approximate (81% of a 200k show/read sample comes back 1-7 ULP off, subnormals parse as 0, and the largest finite double reads as Infinity). Raised issue #218...

mchav · 2026-08-19T15:30:56Z

+    | T.length t <= 18 = case signed decimal t of
+        Right (value, "") -> Just value
+        _ -> Nothing
+    | otherwise = case signed decimal t :: Either String (Integer, T.Text) of


Integer is also way more expensive.

Revrrted. although I think fwiw the Integer path only kicked in for fields longer than 18 chars? like the common path was untouched, but it'll come back with numbers in the parsing PR.

…hanges

skymanbp · 2026-08-19T17:49:45Z

Trimmed this down to the null-handling half per review:

dropNulls is Column -> Column with your call-site shape; the helper is
renamed and private
metrics throw on no compared pairs
Int mean/median, correlation and parsing changes reverted; filed
Int mean/median wrap at 2^63 #216/correlation loses precision on offset low-variance columns #217/readDouble is off by 1-7 ULP; subnormals parse as 0 #218 with repros so they don't get lost
variance n < 2 is still NaN pending the kernel question (thread above)

Full suite green locally.

mchav · 2026-08-20T05:29:07Z

+{- | 'dropNulls', unless the view type @a@ is @Maybe@-headed: a @Maybe@-typed
+view encodes the nulls as values, so the column passes through untouched.
+-}
+dropNullsExceptMaybe :: forall a. (Typeable a) => Column -> Column


This is confusing. Why doesn't drop nulls handle this case. That would be a good thing to put in the comment. Let me read the calm site.

mchav · 2026-08-20T06:42:32Z

 valueProportions expr df
    | null df = throw (EmptyDataSetException "valueCounts")
-    | otherwise = case columnAsVector expr df of
+    | otherwise = case nonNullElements expr df of


This doesn't make sense to me. A column with a null bitmap here should render as a Maybe a since that's how the user should assume when computing. Our null bitmap is an implementation detail that shouldn't drift from the DSL which assumes all Maybes are null bitmapped. And that's gated at column construction. So this nonNullElements function is both superfluous and maybe a little harmful.

Fair. Helper dropped. valueCounts/valueProportions are now back on columnAsVector so a nullable column reads as Maybe a. Although, I think toVector @int on a bitmapped Int column takes the raw branch and ignores the bitmap. Not sure if you want an issue for it?

mchav · 2026-08-20T06:46:31Z

-    | n < 2 = 0 -- or error "variance of <2 samples"
+    | n == 0 = throw $ EmptyDataSetException "variance"
+    -- Sample variance is undefined at n = 1: NaN, not a spurious 0.
+    | n < 2 = 0 / 0


Cool. Maybe to make this PR easier to reason about please add specific examples or behaviour changes in the PR description. Sort of like how a front end engineer adds screenshots to their PRs. A clear before and after of failure modes and their new fixes - they could even be copies of the test examples but they should be something like:

Before:

$ ./scripts/repl.sh ghci> :script dataframe.ghci dataframe> df <- D.readCsv "./data/housing.csv" dataframe> -- the operation

Then after would paste a similar thing.

mchav · 2026-08-20T06:46:46Z

 computeSkewness (SkewAcc n _ m2 m3)
    | n < 3 = 0 -- or error "skewness of <3 samples"
-    | otherwise = (sqrt (fromIntegral n - 1) * m3) / sqrt (m2 ^ (3 :: Int))
+    -- raw sums: g1 = sqrt n * m3 / m2^(3/2)


This comment repeats the line below.

skymanbp added 3 commits August 19, 2026 03:00

Merge branch 'fix/numeric' into numeric-correctness

8fbe69f

mchav requested changes Aug 19, 2026

View reviewed changes

Address review: dropNulls on Column, metrics throw, revert hot-path c…

1aa3329

…hanges

This was referenced Aug 19, 2026

Int mean/median wrap at 2^63 #216

Open

correlation loses precision on offset low-variance columns #217

Open

readDouble is off by 1-7 ULP; subnormals parse as 0 #218

Open

skymanbp requested a review from mchav August 19, 2026 18:03

Shorten or drop inline comments

bd873cc

mchav requested changes Aug 20, 2026

View reviewed changes

Remove nonNullElements; nullable columns read as Maybe

321b3c6

skymanbp requested a review from mchav August 20, 2026 07:41

Uh oh!

Conversation

skymanbp commented Aug 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Before / after

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skymanbp Aug 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skymanbp commented Aug 19, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

skymanbp commented Aug 19, 2026 •

edited

Loading

skymanbp Aug 19, 2026 •

edited

Loading