Conversation
The conversion from a raw bin count to the statistic selected by -Z was spelled out as the same if/else chain in four places (the y-range in pshistogram_fill_boxes, the bar height and the cpt z-value in pshistogram_set_xy_array, and the -I dump). Replace all four with pshistogram_stat_value, where apply_log = false yields the linear statistic that the cpt lookup has always used. Also compute the y-range by walking the bins and tracking the min/max of the statistic itself, rather than taking the min/max of the raw counts and transforming afterwards. That shortcut only works while the statistic is an increasing function of the bin content, which is true for the current -Z modes but not for the per-bin-width normalizations in issue #9212, since -T may hand us bins of unequal width. No change in output for any existing -Z mode, with one exception: with +w and a negative total weight, master's y-range (reported by -I, and the basis of an automatic -R) came out as 0/0 even though the bars and the -I dump were positive percentages. It now spans them. The bars and the -I dump themselves are unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Add three new -Z statistics, appended after the existing 0-5 so no script using the current numbering breaks: 6 = frequency: n / w 7 = probability: n / N [alias: proportion] 8 = density: n / (N * w) where n is a bin's count (or sum of weights under +w), N is the sum of n over all bins, and w is that bin's own width -- which may differ from bin to bin (-T accepts a list or file of edges, and +l/+b build log-spaced arrays). Bar heights, the -I dump, the y-axis range and the -C+b lookup all go through pshistogram_stat_value(), the helper introduced in the prior commit, now width-aware for these three modes. Two things needed more than a new switch case: - -N's normal-distribution overlay scales its curve by "area", the sum of (bin width x count), i.e. an average bin width times N. That suits counts, percent and probability, whose bars grow with each bin's width, but not frequency or density, which have already divided it out: a frequency curve is N*phi(x) and a density curve is phi(x) itself. Neither uses "area", so both stay exact when the bins are unequal, whereas the other modes' curves are only approximate then. - -Q (cumulative) sums raw counts before any -Z transform, so a naive "cumulative density" would reach 1/w instead of 1 -- not a CDF. Reject -Q with -Z6/-Z8 outright; -Z7 (probability) has no such problem since n/N does not depend on bin width, and doubles as a real CDF under -Q. This is the recommended choice, not yet confirmed by the maintainer. -D labels are unchanged: they show each bin's raw count for every -Z type, as documented, and the docs now say so explicitly. Also updates the longopt table (frequency, probability|proportion, density), the -Z, -Q, -C and -D docs in histogram.rst (shared with pshistogram.rst) and the -C usage text, and adds two tests: the l2s long-to-short translation list now covers the three new names, and a new numeric test checks two properties of density under deliberately unequal (log-spaced) bins -- that the bars integrate to exactly 1, and that the automatic y-range equals the true per-bin min/max rather than a rescaling of the raw counts. Modes 0-5 and the eight DVC baseline images are unchanged (RMS 0.0000). No new baseline image for the new modes: registering one needs a dvc push to the DagsHub remote, for which this environment has no credentials. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Claude's comments:
I haven't personally checked in detail whether the restriction on combining -Q with -Z6/-Z8 makes sense. Happy to adjust if maintainers feel differently.
Tested with:
Fixes #9212
Assisted-by: Claude Opus 5 and Claude Sonnet 5, reviewed and revised with Claude Opus 5.5