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
Sorry, something went wrong.
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
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, setupMultiCategory keys 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 handleCategoryOrderDefaults directly in Node across the documented matrix (default, valid/invalid/mixed categoryarray, implicit switch to array, ascending/descending, value-order fallback, numeric labels, ragged rows, empty data) — all outcomes match the PR description, and _fullLayout ends up reflecting what will actually happen (the value-order fallback rewriting categoryorder to trace follows the precedent already in this function for an invalid categoryarray).
- Confirmed the seeding path claim: clearCalc → setCategoryIndex already handles array-valued categories, so _initialCategories holding pairs needs no changes downstream, and sortAxisCategoriesByValue (plots.js:3102) skips non-category axes, 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 named toString previously 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 _categories now skips the match-group trace merge in setupMultiCategory (ax._categories.length === 0 gate) — plausibly fine, but untested; see test comment.
- Baselines: as noted in the description, the three corrected baselines plus the new multicategory-categoryorder baseline 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/categoryarray descriptions now mention multicategory on axes that can't be multicategory (gl3d, polar, carpet) — harmless, but worth knowing it's there.
Generated by Claude Code
Sorry, something went wrong.
| // [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
Sorry, something went wrong.
| // '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
Sorry, something went wrong.
| // 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 + ',' + v1 lookup key collides for labels containing commas (['a', 'b,c'] vs ['a,b', 'c']). That's consistent with how _categoriesMap already 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) for lookup is a nice touch matching the set_convert.js fix; findCategories above still uses {}, which is safe only because it assigns rather than uses in — another argument for the shared helper owning this detail.
Generated by Claude Code
Sorry, something went wrong.
| }); | ||
| }); | ||
|
|
||
| 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. setupMultiCategory merges sibling trace indices only when group && ax._categories.length === 0 (set_convert.js:365). Before this PR that condition always held for multicategory axes, because _initialCategories was never set and clearCalc seeded nothing. Now any non-trace categoryorder pre-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 under matches (with, say, a partial categoryarray so some categories come from the data of each axis) would pin down the intended ordering there.
-
Non-contiguous parents in categoryarray. Nothing stops categoryarray: [['A','x'], ['B','y'], ['A','z']], which puts parent A in two separate runs — the divider/label logic will render A twice. That's arguably the right behavior (the array is honored verbatim), but a test asserting the resulting _categories would document that it's intentional rather than an accident.
Generated by Claude Code
Sorry, something went wrong.
|
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 set_convert.js, modifying the existing structure in place, and it fixes default rendering being wrong — the committed multicategory2 baseline literally encodes the bug (q1, q3, q2). Severity high, code small. With its two unit tests, the three corrected baselines, and the draftlog, this stands alone as an obviously landable PR. The categoryorder/categoryarray support is where the judgement call lives. It's ~110 lines of hand-written src plus attribute docs, schema regen, a new mock/baseline, and most of the test matrix — for an attribute combination that has been a silent no-op on multicategory axes since the type was introduced, apparently without much demand until the downstream report. It also carries most of the ongoing surface: pair-validation semantics, two fallback rules, and the open edge cases from review (matched axes, non-contiguous parents). None of that is badly built — but it's feature-scale weight riding on a bug-fix justification, and it's the part that should be weighed (and possibly deferred) on its own merits rather than inheriting urgency from the fix. If the feature half proceeds, it can also get smaller. Concretely:
Generated by Claude Code |
Sorry, something went wrong.
|
Authored by Claude Code (Fable) Split as discussed above:
Both are drafts. This PR is superseded and can be closed. Generated by Claude Code |
Sorry, something went wrong.
Written with Claude Code
Draft — the three baselines noted below still need regenerating, see Baselines.
The problem
On a multicategory axis 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:
P2 is flipped: b was seen first under P1, so b precedes a under 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…Dec ahead of Jan…Jun for every year:
before 2023 -> Jul Aug Sep Oct Nov Dec 2024 -> Jul Aug Sep Oct Nov Dec Jan Feb Mar Apr May Jun <- supplied Jan..Dec 2025 -> Jan Feb Mar Apr May Jun after 2023 -> Jul Aug Sep Oct Nov Dec 2024 -> Jan Feb Mar Apr May Jun Jul Aug Sep Oct Nov Dec 2025 -> Jan Feb Mar Apr May Jun2023 and 2025 looked correct before only because each happens to be a contiguous slice of that one global ordering.
Separately, categoryorder and categoryarray were never coerced on multicategory axes — handleCategoryOrderDefaults returned early for any non-category type — 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 named toString no longer resolves through Object.prototype.
category_order_defaults.js. Let multicategory axes through, and handle the pair shape:
No new attributes; categoryarray is already data_array. Descriptions updated for both, with test/plot-schema.json regenerated.
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 each categoryorder mode including both fallbacks.
npm run test-jasmine -- axes goes from 398 to 408 passing. The 2 insiderange failures in my sandbox are font-metric tolerances that fail identically on unmodified master.
Regression sweep
Rendered all 1067 non-gl3d/map/geo mocks under master and this branch and diffed each multicategory axis's resolved _categories. 1064 identical, 3 changed — all three corrections:
multicategory2 is the clearest: the mock supplies 2018 q1, q2, q3 and the committed baseline shows q1, q3, q2 — the existing baseline encodes the bug.
multicategory-sorting subplot 2 draws 4/2 from the first trace before 4/1 from the second, so per-parent order is 4/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 multicategory baseline 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
🤖 Generated with Claude Code
https://claude.ai/code/session_01XAGQnaXsViqVPvak39Y4qo