Repository navigation
Conversation
|
Thanks for the PR! Our team will review and follow up. |
chrikrah
left a comment
There was a problem hiding this comment.
@vineoy blocking: with this branch, the discrete-color Gantt example in doc/python/gantt.md:69-76 lists its tasks bottom-up, the opposite of what that page promises.
# df = the three-task Resource frame at doc/python/gantt.md:69-73
>>> px.timeline(df, x_start="Start", x_end="Finish", y="Task").layout.yaxis.categoryarray
None # 85cb798 and main d586d22
>>> px.timeline(df, x_start="Start", x_end="Finish", y="Task", color="Resource").layout.yaxis.categoryarray
('Job C', 'Job B', 'Job A') # 85cb798
None # main d586d22
>>> px.bar(x=[1,2,3,4], y=list("abcd"), color=list("ppqq"), orientation="h").layout.yaxis.categoryarray
('d', 'c', 'b', 'a') # 85cb798
None # main d586d22
The page pairs that call with autorange="reversed" so tasks read top-down. The new default goes through the y reversal at _core.py:639, and the two reversals cancel. A colored horizontal bar flips the same way, even where the groups do not interleave. #3291 asked for the x order to be set internally, and it said nothing about y. Two ways out:
- Apply the default to
xonly. - Keep
y, and store its default so that the reversal at_core.py:639gives back the bottom-up order.
The x-axis fix itself holds: the two data-order tests fail with _core.py from main and pass on the branch.
$ pytest -q tests/test_optional/test_px/test_px.py # 85cb798
3 failed, 331 passed, 4 warnings in 29.64s
$ pytest -q tests/test_optional/test_px/test_px.py # main d586d22
3 failed, 328 passed, 4 warnings in 27.27s
$ cp <main>/plotly/express/_core.py plotly/express/ && pytest -q tests/test_optional/test_px/test_px.py -k "data_order or no_default_category"
2 failed, 1 passed, 331 deselected, 4 warnings in 0.82s
The three failures on both commits are test_render_mode, which needs statsmodels.
@camdecoster would you take fix 1 or fix 2 here?
Address review on plotly#5776: y categoricals stay untouched so the autorange=reversed pattern from doc/python/gantt.md keeps listing tasks top-down instead of double-reversing. Adds guard test that px.timeline and horizontal px.bar with color emit no y categoryarray, while x data-order fix for plotly#3198/plotly#3291 is kept.
|
Thanks @chrikrah for catching the y axis double reversal. I took fix 1 and scoped the default to x only. y categoricals are now untouched, so the autorange reversed pattern in doc/python/gantt.md keeps listing tasks top down. Pushed d6ddc2a. It limits _default_category_orders_for_axes to x with a note on why y is excluded, keeps the x data order fix for #3198 and #3291, adds test_no_default_category_array_for_y_with_color for px.timeline and horizontal px.bar with color, and updates CHANGELOG to say x axis. Verified locally. The x repros still give ('A','B','C','D','E') and ('a','b','c','d'). The y timeline and y bar h cases now give None, matching main. test_px.py gives 3 failed (pre existing test_render_mode statsmodels gap) and 332 passed. Ruff check is clean. @camdecoster please let me know if you would rather have fix 2. |
Address review on plotly#5776: y categoricals stay untouched so the autorange=reversed pattern from doc/python/gantt.md keeps listing tasks top-down instead of double-reversing. Adds guard test that px.timeline and horizontal px.bar with color emit no y categoryarray, while x data-order fix for plotly#3198/plotly#3291 is kept.
d6ddc2a to
16ab376
Compare
chrikrah
left a comment
There was a problem hiding this comment.
@vineoy the Gantt flip is fixed at 16ab376, so I am approving. The y axis gets no default order, and the x data-order fix still holds.
# df = the three-task Resource frame at doc/python/gantt.md:69-73
>>> px.timeline(df, x_start="Start", x_end="Finish", y="Task", color="Resource").layout.yaxis.categoryarray
None # 16ab376 and main 3f8a650
>>> px.bar(x=[1,2,3,4], y=list("abcd"), color=list("ppqq"), orientation="h").layout.yaxis.categoryarray
None # 16ab376 and main 3f8a650
>>> px.bar(x=list("abcd"), y=[1,2,3,4], color=list("pqpq")).layout.xaxis.categoryarray
('a', 'b', 'c', 'd') # 16ab376
None # main 3f8a650
The uncolored timeline stays None on both. Each new test also fails without its part of the change:
$ pytest -q tests/test_optional/test_px/test_px.py # head 16ab376
3 failed, 332 passed, 4 warnings in 33.23s
$ pytest -q tests/test_optional/test_px/test_px.py # main 3f8a650
3 failed, 328 passed, 4 warnings in 28.49s
$ cp <3f8a650>/plotly/express/_core.py plotly/express/ && pytest -q tests/test_optional/test_px/test_px.py -k "data_order or no_default_category"
2 failed, 2 passed, 331 deselected, 4 warnings in 0.86s # both data_order tests
# the x-and-y loop from 85cb798 spliced into _core.py, same -k
1 failed, 3 passed, 331 deselected, 4 warnings in 0.83s # test_no_default_category_array_for_y_with_color
# Python 3.12, pytest 9.1.1; the 3 failures on both commits are test_render_mode, which needs statsmodels
# not run: the rest of tests/test_optional
non-blocking: CHANGELOG.md now has ## [7.1.0] - 2026-09-15 twice, at lines 22 and 24. The new entry sits under Unreleased and an empty 7.1.0 section follows it. Drop the added heading.
@camdecoster from my side this is ready to merge. The y axis can follow in its own PR if you want it.
|
Thanks @chrikrah for verifying. I removed the duplicated 7.1.0 heading in CHANGELOG, so the new entry now sits directly under Unreleased followed by a single 7.1.0 section. One note. Your review reads as an approval in text but GitHub recorded it as a comment, so the PR still shows review required. Could you mark it approved when you get a chance, if you are happy with the current state. |
chrikrah
left a comment
There was a problem hiding this comment.
@vineoy I'd merge 55d5c75. Since 16ab376 it only drops the second ## [7.1.0] line, so CHANGELOG.md reads Unreleased, then one 7.1.0 section.
$ git diff --stat 16ab376 55d5c75
CHANGELOG.md | 2 --
$ pytest -q -p no:cacheprovider tests/test_optional/test_px/test_px.py # head 55d5c75, Python 3.12, pytest 9.1.1
3 failed, 332 passed, 4 warnings # the 3 are test_render_mode, statsmodels not installed
$ cp <3f8a650>/plotly/express/_core.py plotly/express/ && pytest -q -p no:cacheprovider tests/test_optional/test_px/test_px.py -k "data_order or no_default_category"
2 failed, 2 passed, 331 deselected # both data_order tests
$ git merge-tree --write-tree 3f8a650 55d5c75 # main has not moved
296ca7b5d8f45aba1536e1c72609fcada2099fac # exit 0, no conflict
The badge won't move for this, though. My account has no write access here, so GitHub leaves my review out of the required count whatever flag it carries, and only a maintainer clears it. @camdecoster nothing is open on my side. Would you rather see the y axis default handled here or in a follow-up PR?
|
Thanks @chrikrah for the extra verification. @camdecoster on the y axis question, my preference is a follow up PR so this one stays minimal and scoped to the x axis fix for #3198 and #3291. Happy to pick up the y axis separately if you want it. This PR is ready from my side. |
KoolADE85
left a comment
There was a problem hiding this comment.
Thanks for the submission here!
In addition to the breaking change that I found, there are some formatting issues revealed by the CI jobs that will need to be fixed. Please also resolve the trivial CHANGELOG conflict.
Cheers!
Address review on plotly#5776: facet axes must keep listing their own categories (the matches=None technique in doc/python/facet-plots.md) and marginal axes span the y dimension, so the data-wide default must not reach any axis of a faceted or marginal figure. Adds the unlinked facet axes test plus a subplot guard test, limits the changelog entry, and applies ruff format to the touched files.
Keep main's new Unreleased changelog entries and place the Plotly Express x-axis entry at the end of the Fixed list.
Link to issue
Fixes #3198, fixes #3291.
Description of change
When Plotly Express splits data into several traces (e.g. via
color), plotly.js orders categories by first appearance across traces (defaultcategoryorder). Because each trace only carries its own rows (None-padded), that order can differ from the order in the underlying data, contradicting the documented behavior that Plotly Express lays out categorical data in the order in which it appears in the data.This change records the data-appearance order of the string categorical
xcolumn inmake_figure(new_default_category_orders_for_axeshelper inplotly/express/_core.py), but only when the figure has a single subplot, the data is actually split into more than one trace, and the user did not supply an explicitcategory_ordersentry. The existingset_cartesian_axis_optsthen emitscategoryorder=array+categoryarray, so:px.line(x=[...], y=[...], color=[...])from Bug when using categorical axes and colors in plotly.express #3198 now yieldscategoryarray=('A', 'B', 'C', 'D', 'E')instead of trace orderA, B, D, E, Cpx.bar(df, x='x', y='y', color='z')from Plotly Express doesn't force category orders on axes #3291 now yields('a', 'b', 'c', 'd')Explicit user
category_orders, single-trace figures, figures with facets (their axes keep listing their own categories, so the independent-axes technique indoc/python/facet-plots.mdstill works), figures with marginals (their extra subplots span the y dimension), and numeric/datetime axes are untouched.Testing strategy
tests/test_optional/test_px/test_px.py: data-order assertion for Bug when using categorical axes and colors in plotly.express #3198 and Plotly Express doesn't force category orders on axes #3291 repros, guard tests that single-trace and numeric-axis figures get nocategoryarray, the unlinked facet axes test from the review, and a guard test that faceted and marginal figures get nocategoryarrayon any axis.CHANGELOG.mdentry added under Unreleased/Fixed.test_px.py(334 passed; 3test_render_modefailures are pre-existing missing-statsmodelsenv gaps, verified identical on clean main);test_px_functions/test_facets/test_px_wide/test_px_inputfailure sets identical to clean main (pre-existing env gaps: pyarrow timezones); pinnedruff==0.11.12format --checkclean on all files.