Reset forced minimum tick spacing on every calc pass - #7950
Open
Jaybhade wants to merge 2 commits into
Open
Conversation
setConvert cleared ax._minDtick / ax._forceTick0 so each calc pass would start over, but the cleanup never had any effect: setConvert runs while supplyDefaults builds the new _fullLayout, where the keys do not exist yet, and relinkPrivateKeys then copies the old values back onto it. Axes.minDtick treats 0 as "forcing cancelled", so the 0 written by whichever figure was drawn first survived every later update and vetoed the forcing for every figure after it. Reacting from a scatter to a box plot lost the one-tick-per-box spacing, while newPlot of the same figure kept it. Move the reset into ax.clearCalc, the axis' own per-calc-pass reset, which doCalcdata runs for every axis before any cross-trace calc.
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
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.
The bug
Plotly.reactcan draw an axis with different ticks thanPlotly.newPlotof the exact same figure. Same forrestyle,addTracesand friends — anything that updates a graph div in place.Box, violin, candlestick and ohlc traces ask the position axis for a minimum tick spacing, so that each box gets a tick of its own instead of ticks at meaningless in-between positions. In the second case that forcing is silently lost.
It is most obvious on a date axis. Five daily candles at
width: 1000, reached by reacting from a line chart over the same dates:Every second tick falls in the gap between two candles, and the axis now starts on the day before the data.
Cause
Axes.minDtickkeeps its state inax._minDtick/ax._forceTick0and distinguishes three cases:undefined(nothing forced yet — adopt this trace's spacing), a positive number (a forcing is in effect), and0(forcing cancelled, e.g. by non-grouped bars or by scatter/heatmap, and sticky so a later trace can't reinstate it).The reset back to
undefinedbetween passes was at the bottom ofsetConvert, but it has never had any effect on a real axis:setConvertruns whilesupplyDefaultsbuilds the new_fullLayout, where those keys don't exist yet, andrelinkPrivateKeysthen copies the old values back onto it. So whichever figure the graph div held first decides the forcing forever after.Plotly.newPlotisn't affected because it starts from an empty_fullLayout.doCalcdataalready handles the identical relink staleness for shared color axes ("clear relinked cmin/cmax values in shared axes to start aggregation from scratch"). This moves the reset intoax.clearCalc()— the axis' own per-calc-pass reset, whichdoCalcdataruns for every axis, and always before anycrossTraceCalc.One existing expectation changed
plot_api_test.js→ "updates box position and axis type when it falls back to name" asserted that a single box restyled tox0: 12.3ends up with ticks['12', '12.5']. That was the stale value.Plotly.newPlotof that same figure produces a single tick at12.3, on master as well as here, so the change makesrestyleagree withnewPlot:newPlotboxx0: 12.312.312.3restyletox0: 12.312,12.512.3reacttox0: 12.312,12.512.3Testing
axes_test.js→ "should not carry over the forced minimum tick spacing of the previous figure". It fails on master (Expected 0 to be 1) and passes here.npm run test-jasmine -- --nowatch: 7000 specs, and the failure set is identical to master's on this machine (a batch of font-metric, WebGL, drag/touch and@flakytests fail either way; the two extra failures in my run pass when their suites are run alone).npm run lintandnpm run test-syntaxare clean.test-image/test-export. Those baselines shouldn't be able to move: they're all singlenewPlotcalls, which never have a previous_fullLayoutto relink from.