Weight MicroDataFrame.cov() and .corr() - #330
Conversation
MicroDataFrame.cov() and .corr() delegated to pandas on the unweighted frame, so a weighted frame disagreed with its own columns. Each cell now uses the MicroSeries estimator with the frame's weights over the rows where both columns are present. The pair numerics move to module-level helpers shared by both classes. corr() accepts only method="pearson", as on MicroSeries. Fixes #327 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Checked out Edge cases behave as the description says: NaN rows drop per pair (3.5238, matching the replicated sample with that row removed), zero-weight rows are omitted, negative weights raise, Four findings, the first one blocking. 1. This collides with #324, and whichever merges second breaks CI. My #324 documents these two methods as unweighted — that was the correction you asked for in your second review — and So the interaction is real in both directions. The note in your description that "#324 rows already say frequency-weighted" is out of date — they said that in the first version, and I changed them to "unweighted" with a warning box pointing at #327 after your review. The clean order is this PR first, then I revert #324's warning box and unweighted labels and flip that assertion back to the weighted expectation before it merges. If #324 goes first instead, this PR needs the same edit. Either way one of us edits after the other lands; I am happy for it to be me, since your change is the substantive one and mine is documentation. 2. The performance figures understate the gap. I measured 50,000 × 100 at 3.33 s weighted against 0.01 s unweighted, not 0.4 s — so roughly 300×, not 7×. Absolute time matches what you reported, so the conclusion is unchanged, but the unweighted baseline looks like it included frame construction. Worth stating the real ratio, since 3 s on a 100-column frame is the kind of thing someone hits in a loop. 3. The Externally is the open question. 4. Minor. The helper extraction into Approving on the substance. Holding the formal approval only until we agree the merge order for finding 1, so neither of us lands a red |
|
Thanks for running it rather than reading it. The four in order. 1. Merge order: agreed, this PR first. My round-4 note on #324 said the opposite; it was written before #324 went red on the signature test, and your way round is cleaner now. This lands, then #324 drops the warning box, relabels the two rows as frequency-weighted, and flips the 2. Performance: both baselines are right, they measure different pandas paths. pandas uses 3. External callers. Checked 4. Hoisting the means. Only valid when the pair has no missing values, since pairwise deletion changes each pair's mean. That is exactly the case where a single centred matrix product replaces the loop and closes most of the complete-data gap in finding 2. Follow-up rather than this PR, as you say. |
|
All four resolved. No objection to merging — this should go in first, and I will do the #324 edit after it lands. On finding 2, your explanation is right and mine was incomplete. I reproduced it on 50,000 × 100: pandas Finding 4 — agreed, and your qualification is the important half: hoisting is only valid when the pair has no missing values, because pairwise deletion changes each pair's mean. That is also exactly the complete-data case where a single centred matrix product replaces the loop, so the follow-up is worth filing when someone hits the 3 s. Finding 3 — thank you for checking Finding 1 — I will take the #324 edit: drop the warning box, relabel both rows frequency-weighted, and flip the assertion to the replicated value. Ping me when this merges. |
#330 merged, so MicroDataFrame.cov and corr now use the weights. Drops the warning box and folds both back into the weighted aggregation table, with the descriptions restored to frequency-weighted, and flips the assertion in test_documented_weight_behaviour_holds to the replicated sample. It also now asserts each frame cell equals the corresponding MicroSeries value, which is the property #330 introduced. The CI Lint failure was docformatter rather than ruff - make lint runs both, and the new _docs.py needed its docstrings rewrapped. Merges main in, so the branch carries #330 and #331.
* Add an API reference to the documentation The documentation had no API page, which is the largest gap a JOSS reviewer assessing documentation would find. Lists every weighted estimator and weight-handling method on both classes with its signature and a one-line description, taken from the docstrings, plus the estimator conventions that distinguish these from naive weighted equivalents. Registered in both docs/myst.yml and docs/_toc.yml, which are kept in step until the duplicate configs are reconciled. * Document the weighted estimators, and check coverage both ways Per review, the page listed the named estimators and the weight helpers but omitted the weighted aggregation methods and the weight-preserving overrides - sum, mean, median, quantile, var, std, cov, corr, rank, groupby, merge, drop and the rest. Those are the core of both things the page says it covers, and the README's feature list promises them. The coverage check only ran one way, from the page to the package, which is why it passed. It is now a test that runs in both directions, so a new public method that is not documented fails. Also adds docstrings to quintile_rank, quartile_rank and percentile_rank rather than hand-writing their description cells, and corrects the set_weights annotation from np.array, a function, to np.ndarray. The changelog fragment becomes .changed so this releases as a patch. * Correct three wrong descriptions on the API page MicroDataFrame.cov and corr do not use the weights - they call pandas on the plain frame - but the rows described them as frequency-weighted and sat under Weighted aggregation. Verified: with w=[1,1,1,5] the frame gives 3.1667, the unweighted pandas value, while the replicated sample and MicroSeries.cov both give 3.1786. They now carry a warning pointing at #327 and say plainly that they are unweighted. equals compares weights on both classes - both return equal_values and equal_weights - and the rows claimed the opposite. The set_weights row still rendered '<built-in function array>' because the page was not regenerated after the annotation fix, and quantile had the same artifact from an unfixed 'q: np.array'. Both annotations are now np.ndarray in source, in microseries.py and microdataframe.py, and the rows regenerated. Also marks values as an attribute rather than showing a call signature, skips the coverage test when docs/ is absent so the packaged tests pass, and drops an unused import and parameter. * Correct cumsum, and pin the hand-written claims in tests Audited every description on the page that was written by hand rather than taken from a docstring, since that is where all three errors found in review were. cumsum was the remaining one. It returns a plain pandas Series and warns that the weights have been applied and cannot be reused, so it is the one method in the weight-preserving section that does not preserve them. The page now says so. The rest hold: clip, round, sqrt, copy and astype carry weights through; repeat repeats them alongside the values; merge, reset_index and drop keep them aligned, and drop(index=) drops the matching weights; groupby returns the MicroSeriesGroupBy and MicroDataFrameGroupBy wrappers. Those checks are now a test rather than something I ran once, covering the claims no docstring enforces: the unweighted frame cov and corr, the weighted MicroSeries cov, equals comparing weights, cumsum dropping them, and repeat carrying them. * Regenerate the rows the source outran, and test that it cannot recur The three rank rows were still blank: they got docstrings in the first round but the page was never regenerated, the same miss as set_weights in the second. Patching individual rows by hand is what kept producing this, so the rows are now regenerated from the live signatures and docstrings, and two tests hold the page to the code. test_no_row_is_missing_its_description fails on any blank description cell. test_signatures_match_the_live_ones compares every rendered signature against inspect.signature, attributing each row to the class whose heading it falls under, since several names exist on both. The second test immediately found a real error: the MicroDataFrame set_weights row carried the MicroSeries signature, because the earlier regex fix matched both rows. Regenerated. * Generate the API page from a committed script, version-stably The script that produced docs/api.md was run by hand and lived outside the repo, which is why the source outran the page four times in review. It is now docs/build_api.py: the prose is held verbatim, the tables are generated from the live classes, and the hand-written descriptions that are not docstrings (cumsum, merge, reset_index, drop, equals, copy, groupby, clip, round, repeat, sqrt, and the two unweighted cov/corr rows) are an explicit override table so regeneration cannot lose them. str(inspect.signature(...)) was never safe to compare as text: pandas 2 renders a Series annotation as pandas.core.series.Series and pandas 3 renders it as pandas.Series, so a page generated under one fails CI on the other's jobs, and from Python 3.14 Optional[X] reprs as X | None. microdf/_docs.py renders signatures from parameter names, kinds and defaults, strips module qualifiers and puts every union into one form. The generator and the test both import it, so the page cannot disagree with the test, and the same committed page passes under pandas 2.3 and 3.0 and on Python 3.9 through 3.14. * Follow #330: cov and corr are weighted, and satisfy docformatter #330 merged, so MicroDataFrame.cov and corr now use the weights. Drops the warning box and folds both back into the weighted aggregation table, with the descriptions restored to frequency-weighted, and flips the assertion in test_documented_weight_behaviour_holds to the replicated sample. It also now asserts each frame cell equals the corresponding MicroSeries value, which is the property #330 introduced. The CI Lint failure was docformatter rather than ruff - make lint runs both, and the new _docs.py needed its docstrings rewrapped. Merges main in, so the branch carries #330 and #331.
Per review: #291 and #330 landed, so cov and corr are frequency-weighted on both classes and the paragraph saying they fall through to pandas with a warning is out of date. No method now returns an unweighted result behind a warning; the warnings that remain guard values and to_numpy, which deliberately hand back plain data. Takes the suggested wording for the classification sentence.
Merges main, so the branch carries the weighted MicroDataFrame cov and corr from #330 and the API reference from #324. Without it a reviewer checking out this branch would see behaviour the paper does not describe. Corrects the replication paragraph: the ACS and CPS ASEC use successive-difference replication, which is the 4/R scale the paper quotes. Fay's variant of BRR is a different scheme with a 1/(R(1-k)^2) scale, and replication.py already keeps the two apart. Refreshes the state of the field. R's convey is the closest existing equivalent to microdf's estimator set and was absent; the table said R survey had limited inequality measures, which is true of survey alone and misleading once convey exists. samplics is now archived in favour of svy, and the paper said neither. Adds what DescrStatsW does cover. Adds a paragraph placing this work against the published policyengine paper, since a reviewer will otherwise ask why the dependency is not covered there.
2 participants
Fixes #327.
MicroDataFrame.cov()and.corr()returned plain pandas results: the overrides calledpd.DataFrame(self).cov(...)on the unweighted frame, so a weighted frame disagreed with its own columns. In the issue's examplem.cov().loc["x", "y"]was 3.1667 whilem.x.cov(m.y)was 3.1786.Change
MicroSeries.cov/.correstimator with the frame's weights, over the rows where both columns are present and the weight is positive. Integer weights matchpandas.DataFrame.covon the replicated sample, and each cell equals the correspondingframe[a].cov(frame[b])exactly, because it runs the same code.MicroSeries.cov/.corrinto module-level helpers inmicroseries.py(_pair_options,_validate_frequency_weights,_usable_pair,_weighted_covariance,_weighted_correlation) that both classes call.MicroSeriesbehaviour is unchanged; its existing cov/corr tests pass untouched.pandas.DataFramewith no row weights, andattrs,flagsand the columns name still carry over, as the earlier review fix required.cov(min_periods=None, ddof=1, numeric_only=False)andcorr(method="pearson", min_periods=1, numeric_only=False).min_periodscounts usable row pairs, not weight, as onMicroSeries.numeric_only=Falseconverts with the same call pandas uses, so non-numeric columns raise pandas' own error.One behaviour removal
corr(method="spearman"),"kendall"and callables previously delegated to pandas and returned an unweighted matrix. They now raiseValueError("weighted correlation only supports method='pearson'"), the same asMicroSeries.corr. A weighted rank correlation is a separate feature; the unweighted one ispd.DataFrame(frame).corr(method=...). The fragment isfixed; treating this asbreakinginstead would make it a major bump. Framed plainly: this removes a working code path from a published package (microdf-pythonon PyPI). The one known external importer,PSLmodels/scf, calls neither.corr()nor.cov(). Whether that warrantsbreakingis Max's call.Tests
test_weight_propagation.pythat pinned the unweighted values now check against the frequency-replicated sample, keeping the plainness and metadata assertions. That oracle drops missing rows per pair because pandas ignoresddofas soon as any value is missing: it leaves thenp.covpath for itsnancorrkernel.test_weighted_cov_corr.py: replicated-sample match for ddof 0, 1 and 2; exact cell equality withMicroSeries; pairwise-complete rows; zero-weight rows omitted; invalid weights rejected;min_periodson rows, not weight; diagonal and constant columns; insufficient weight total and an empty frame; option validation.Cost
Pairwise over columns in numpy. On 50,000 rows by 100 columns the weighted matrix takes about 3.4 s whether or not values are missing. pandas takes 0.008 s when nothing is missing, because it uses
np.covthrough BLAS, and 0.37 s as soon as one value is missing, because it switches to its pairwisenancorrkernel. So the gap is roughly 430 times on complete data and 9 times on data with any missing values. 20 columns takes 0.14 s. A matrix-product fast path for pairs with no missing values would close most of the complete-data gap and is a follow-up, not this PR.Docs
#324 currently documents these two methods as unweighted, under a warning box pointing at #327, and pins that in
test_documented_weight_behaviour_holds. Agreed order with Vahid: this PR lands first, then #324 drops the warning box, relabels the two rows as frequency-weighted and flips that assertion to the replicated-sample value before it merges.🤖 Generated with Claude Code