Fix multicategory axis ordering and support categoryorder / categoryarray - #7929
Fix multicategory axis ordering and support categoryorder / categoryarray#7929chriddyp wants to merge 2 commits into
Conversation
Second-level categories on a multicategory axis shared one global
ordering keyed on where each label first appeared anywhere in the data,
so every first-level category rendered the same child sequence
regardless of its own data order. With x = [['2023','2024'], ...] and
2023 contributing Jul-Dec first, 2024 rendered Jul-Dec then Jan-Jun even
though it was supplied Jan-Dec.
`setupMultiCategory` now tracks the child first-appearance index per
parent, so each group keeps the order found in its own data. The lookup
objects are prototype-less, so a category named 'toString' no longer
resolves through Object.prototype.
`categoryorder` and `categoryarray` were also never coerced on
multicategory axes - `handleCategoryOrderDefaults` returned early for any
non-category type - so setting them was a silent no-op. They are now
honoured:
- 'trace' (default) keeps the per-parent data order
- 'array' takes `categoryarray` as [first-level, second-level] pairs;
malformed entries are dropped, and an array holding no valid pair
falls back to 'trace'
- 'category ascending'/'descending' sort the pairs by label
- ordering by aggregated value ('total ascending', ...) is not
implemented for these axes and falls back to 'trace' rather than being
accepted and silently ignored
Three existing baselines encode the old order and need regenerating:
multicategory-sorting, multicategory-y and multicategory2. In
multicategory2 the data supplies 2018 q1, q2, q3 and the current
baseline shows q1, q3, q2.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
chriddyp
left a comment
chriddyp
left a comment
There was a problem hiding this comment.
Authored by Claude Code (Fable)
Review summary
Reviewed with particular attention to whether this is a root-cause fix or scattered special-casing. Short version: the core fix is the right shape, and the defaults work extends the existing module symmetrically rather than patching around it — with a few organizational improvements I'd want before this lands (inline comments).
What I verified
- Re-derived the bug: on
master,setupMultiCategorykeys second-level order on first appearance anywhere, so every parent shares one global child order. Sorting by(parent rank, child-rank-within-parent)is the correct semantics, and per-parent maps are the right mechanism. - Exercised
handleCategoryOrderDefaultsdirectly in Node across the documented matrix (default, valid/invalid/mixedcategoryarray, implicit switch toarray, ascending/descending, value-order fallback, numeric labels, ragged rows, empty data) — all outcomes match the PR description, and_fullLayoutends up reflecting what will actually happen (the value-order fallback rewritingcategoryordertotracefollows the precedent already in this function for an invalidcategoryarray). - Confirmed the seeding path claim:
clearCalc→setCategoryIndexalready handles array-valued categories, so_initialCategoriesholding pairs needs no changes downstream, andsortAxisCategoriesByValue(plots.js:3102) skips non-categoryaxes, so the defaults-time fallback is the only guard needed. - The prototype-safety fix (
Object.create(null)) addresses a real pre-existing bug: a child label namedtoStringpreviously hit'toString' in {}→ true and never got an index, producing NaN comparisons in the sort.
On the "well organized vs. patched" question
The category_order_defaults.js changes are structured the way this module wants to grow: getAxData extracted instead of duplicated, findCategoryPairs as a named parallel to findCategories, small named predicates. The isMultiCategory branches each sit at a genuine semantic fork, not sprinkled guards. The two places where I think it falls short of the bar are duplication rather than structure — the VALUE_ORDER_RE literal copied from plots.js, and the pair-traversal logic existing in both findCategoryPairs and setupMultiCategory — see inline comments for concrete consolidations (cartesian/constants.js for the regex; a shared pair-iteration helper for the traversal).
The one place where the implementation (not the semantics) could be meaningfully simpler is setupMultiCategory itself: the fix grafts a second level of index maps onto the old flat-list-plus-sort shape, when the per-parent grouping can be the primary structure and the sort dropped entirely. Details and an equivalence-checked sketch inline.
Open items before undraft
- Matched axes: pre-seeding
_categoriesnow skips the match-group trace merge insetupMultiCategory(ax._categories.length === 0gate) — plausibly fine, but untested; see test comment. - Baselines: as noted in the description, the three corrected baselines plus the new
multicategory-categoryorderbaseline still need generating by a maintainer, so image CI will stay red until then. - Draftlog, schema regeneration, and attribute-description updates all follow repo conventions. Note the shared
categoryorder/categoryarraydescriptions now mention multicategory on axes that can't be multicategory (gl3d, polar, carpet) — harmless, but worth knowing it's there.
Generated by Claude Code
| // [cnt, {$cat: index}] for the first (parent) level | ||
| var seen0 = [0, Object.create(null)]; | ||
| // {$parentCat: [cnt, {$cat: index}]} for the second (child) level, | ||
| // tracked *per parent* so that each parent keeps the child order | ||
| // found in its own data rather than sharing one global order | ||
| var seen1 = Object.create(null); |
There was a problem hiding this comment.
Authored by Claude Code (Fable)
The per-parent semantics are right, but the implementation keeps the old code's shape — flat row list + sort against lookaside index maps — and grafts a second level of maps onto it (seen0, seen1, and a comparator that has to explain why indexing seen1[a[0]] is safe). Since setCategoryIndex already dedups, the sort is doing more work than the problem needs. Consider making the per-parent grouping the primary structure and dropping the sort entirely:
// parents in first-appearance order
var parents = [];
// {$parentCat: {seen: {$childCat: 1}, children: [childCat in first-appearance order]}}
var childrenOf = Object.create(null);
// in the trace loop, replacing the seen0/seen1 bookkeeping:
if(!(v0 in childrenOf)) {
childrenOf[v0] = {seen: Object.create(null), children: []};
parents.push(v0);
}
var c = childrenOf[v0];
if(!(v1 in c.seen)) {
c.seen[v1] = 1;
c.children.push(v1);
}
// replacing list.sort + the setCategoryIndex loop:
for(i = 0; i < parents.length; i++) {
var children = childrenOf[parents[i]].children;
for(j = 0; j < children.length; j++) {
setCategoryIndex([parents[i], children[j]]);
}
}list becomes unnecessary (it's only consumed by the setCategoryIndex loop), it's O(n) instead of O(n log n), and "each parent keeps the child order found in its own data" is expressed by the data structure instead of a comment on the comparator. I ran both versions against 200+ randomized row sets plus the duplicate-row and toString edge cases — the resulting _categories are identical, including the interaction with categories pre-seeded from _initialCategories.
Generated by Claude Code
| // 'total ascending', 'median descending', ... - ordering by aggregated value, | ||
| // which `sortAxisCategoriesByValue` only implements for 'category' axes. | ||
| // Mirrors `sortAxisCategoriesByValueRegex` in plots.js | ||
| var VALUE_ORDER_RE = /(total|sum|min|max|mean|geometric mean|median) (ascending|descending)/; |
There was a problem hiding this comment.
Authored by Claude Code (Fable)
Please don't leave this as a second copy of the literal — if the two regexes ever drift, categoryorder values would be accepted here that sortAxisCategoriesByValue never implements (or vice versa), and the failure would be silent.
The PR description cites a require cycle as the reason for duplicating, but there's an existing home that avoids one: src/plots/cartesian/constants.js already holds shared regexes and only requires lib/regex, so both plots.js and this module can pull it from there:
// constants.js
CATEGORY_ORDER_BY_VALUE_REGEX: /(total|sum|min|max|mean|geometric mean|median) (ascending|descending)/,That keeps a single source of truth for "which orders are value-aggregations".
Generated by Claude Code
| // multicategory variant: returns the unique [parent, child] pairs found in the | ||
| // data, which is what `_categories` holds for these axes | ||
| function findCategoryPairs(ax, opts) { | ||
| var dataAttr = opts.dataAttr || ax._id.charAt(0); | ||
| var axData = getAxData(ax, opts); | ||
| var lookup = Object.create(null); | ||
| var list = []; | ||
| var i, j; | ||
|
|
||
| for(i = 0; i < axData.length; i++) { | ||
| var arrayIn = axData[i][dataAttr]; | ||
| if(!isArrayOrTypedArray(arrayIn) || | ||
| !isArrayOrTypedArray(arrayIn[0]) || | ||
| !isArrayOrTypedArray(arrayIn[1]) | ||
| ) continue; | ||
|
|
||
| var len = Math.min(arrayIn[0].length, arrayIn[1].length); | ||
|
|
||
| for(j = 0; j < len; j++) { | ||
| var v0 = arrayIn[0][j]; | ||
| var v1 = arrayIn[1][j]; | ||
|
|
||
| if(isValidCategory(v0) && isValidCategory(v1)) { | ||
| var key = v0 + ',' + v1; | ||
| if(!(key in lookup)) { | ||
| lookup[key] = 1; | ||
| list.push([v0, v1]); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return list; | ||
| } |
There was a problem hiding this comment.
Authored by Claude Code (Fable)
On the review note about this duplicating setupMultiCategory's traversal: I'd keep the two call sites (defaults-time vs calc-time genuinely differ on how row length is known), but the part worth unifying is the row-walk itself — the "given a 2D coordinate array, visit each valid [parent, child] pair" loop, which encodes the validity rules in both places. A small shared helper, e.g.
// e.g. in lib/ or a cartesian helper module
function forEachValidPair(arrayIn, len, fn) { ... }would let each caller supply its own len (trace._length || Lib.minRowLength(arrayIn) there, Math.min of row lengths here) while keeping isValidCategory-pair semantics in one place. isValidCategory itself is now defined in three modules (set_convert.js, axes.js if I recall, and here) — worth folding into the same helper module while you're at it.
Two smaller notes on this function as written:
- the
v0 + ',' + v1lookup key collides for labels containing commas (['a', 'b,c']vs['a,b', 'c']). That's consistent with how_categoriesMapalready stringifies pairs, so I don't think it needs fixing here — but a one-line comment saying the collision is intentional/consistent would save the next reader the analysis. Object.create(null)forlookupis a nice touch matching theset_convert.jsfix;findCategoriesabove still uses{}, which is safe only because it assigns rather than usesin— another argument for the shared helper owning this detail.
Generated by Claude Code
| }); | ||
| }); | ||
|
|
||
| describe('on multicategory axes', function() { |
There was a problem hiding this comment.
Authored by Claude Code (Fable)
Good coverage of the ordering modes and both fallbacks. Two cases I'd add:
-
Matched axes.
setupMultiCategorymerges sibling trace indices only whengroup && ax._categories.length === 0(set_convert.js:365). Before this PR that condition always held for multicategory axes, because_initialCategorieswas never set andclearCalcseeded nothing. Now any non-tracecategoryorderpre-seeds_categories, so the merge branch is skipped — a behavior change on a path this suite doesn't touch. A test with two multicategory axes undermatches(with, say, a partialcategoryarrayso some categories come from the data of each axis) would pin down the intended ordering there. -
Non-contiguous parents in
categoryarray. Nothing stopscategoryarray: [['A','x'], ['B','y'], ['A','z']], which puts parentAin two separate runs — the divider/label logic will renderAtwice. That's arguably the right behavior (the array is honored verbatim), but a test asserting the resulting_categorieswould document that it's intentional rather than an accident.
Generated by Claude Code
|
Authored by Claude Code (Fable) Follow-up: sizing the change against the problemA maintainer heuristic worth applying here explicitly: the most common failure mode of contributed PRs is a lot of new code for a relatively niche problem. Judged that way, this PR is really two changes with very different size-to-severity ratios, and I'd recommend splitting them. The bug fix earns its weight easily. The per-parent ordering correction is ~25 lines in The If the feature half proceeds, it can also get smaller. Concretely:
Generated by Claude Code |
|
Authored by Claude Code (Fable) Split as discussed above:
Both are drafts. This PR is superseded and can be closed. Generated by Claude Code |
2 participants
Written with Claude Code
Draft — the three baselines noted below still need regenerating, see Baselines.
The problem
On a
multicategoryaxis the second-level categories share one global ordering, keyed on where each label first appears anywhere in the data. Every first-level category therefore renders the same child sequence, regardless of its own data order.Minimal case — data order is
P1/b, P1/a, P2/a, P2/b:P1/b P1/a P2/a P2/bP1/b P1/a P2/b P2/aP2is flipped:bwas seen first underP1, sobprecedesaunder every parent.Real-world shape — months under years, rows supplied in strict chronological order (2023 Jul–Dec, 2024 Jan–Dec, 2025 Jan–Jun). 2023 contributes Jul–Dec first, which pins
Jul…Decahead ofJan…Junfor every year:2023 and 2025 looked correct before only because each happens to be a contiguous slice of that one global ordering.
Separately,
categoryorderandcategoryarraywere never coerced on multicategory axes —handleCategoryOrderDefaultsreturned early for any non-categorytype — so setting them was a silent no-op with no way to work around the ordering above.Reported downstream at plotly/dash-ai-analyst#171.
The fix
set_convert.js—setupMultiCategory. Track the child first-appearance index per parent instead of globally, and sort by(parent rank, child rank within that parent). The lookup objects are now prototype-less, so a category namedtoStringno longer resolves throughObject.prototype.category_order_defaults.js. Let multicategory axes through, and handle the pair shape:trace(default) — per-parent data order, as abovearray—categoryarrayentries are[first-level, second-level]pairs. Malformed entries are dropped; an array holding no valid pair falls back totrace. Categories absent fromcategoryarrayfollow in trace order, matching howcategoryaxes already behavecategory ascending/category descending— sort the pairs by labeltotal ascending, …) is not implemented for these axes —sortAxisCategoriesByValueskips non-categoryaxes, and interleaving children across parents would break the grouping brackets anyway. Rather than accept it and silently do nothing, it now falls back totraceNo new attributes;
categoryarrayis alreadydata_array. Descriptions updated for both, withtest/plot-schema.jsonregenerated.Tests
Visual — new mock
test/image/mocks/multicategory-categoryorder.json, four panels over identical data so each ordering is distinguishable. Data is supplied as 2023 → Q4, Q3 and 2024 → Q2, Q1, Q4, Q3, so trace order is deliberately not alphabetical and all four panels differ:I verified this renders correctly in Chromium against a bundle built from this branch — year brackets intact, each panel distinct — but I can't attach screenshots through the API, so the baseline PNG will be the first rendering committed here.
Unit — 10 new specs in
test/jasmine/tests/axes_test.js: per-parent ordering, prototype-name safety, and eachcategoryordermode including both fallbacks.npm run test-jasmine -- axesgoes from 398 to 408 passing. The 2insiderangefailures in my sandbox are font-metric tolerances that fail identically on unmodifiedmaster.Regression sweep
Rendered all 1067 non-gl3d/map/geo mocks under
masterand this branch and diffed each multicategory axis's resolved_categories. 1064 identical, 3 changed — all three corrections:multicategory22018/q1 2018/q3 2018/q22018/q1 2018/q2 2018/q3multicategory-y2018/q1 2018/q3 2018/q22018/q1 2018/q2 2018/q3multicategory-sorting4/1 4/2 … 6/1 6/24/2 4/1 … 6/2 6/1multicategory2is the clearest: the mock supplies2018 q1, q2, q3and the committed baseline showsq1, q3, q2— the existing baseline encodes the bug.multicategory-sortingsubplot 2 draws4/2from the first trace before4/1from the second, so per-parent order is4/2, 4/1.Baselines — needs a maintainer
Four baselines need generating: the three above, plus the new mock. I did not commit them. My sandbox's kaleido rendering does not match CI's — regenerating the untouched
multicategorybaseline as a control produced 10910 differing pixels (max channel delta 205), i.e. font rendering differs, so any baseline I generated would be wrong in a way unrelated to this change.Happy to push them if a maintainer would rather paste the generated PNGs, or to split the three baseline updates into their own commit.
Notes for review
findCategoryPairsduplicates a little ofsetupMultiCategory's traversal. It runs at defaults time, before calc, wheretrace._lengthisn't available yet — henceMath.minon the two row lengths rather thanLib.minRowLength. Happy to factor it out if you'd prefer.VALUE_ORDER_REmirrorssortAxisCategoriesByValueRegexinplots.js. I kept them as separate literals to avoid a require cycle and left a comment on each; exporting one from a shared module would also work._initialCategoriesseeding inclearCalcalready handled array-valued categories —setCategoryIndexstringifies pairs to"parent,child"for_categoriesMapand pushes the array onto_categories. That mechanism needed no change; multicategory simply never reached it.🤖 Generated with Claude Code
https://claude.ai/code/session_01XAGQnaXsViqVPvak39Y4qo