Bar chart X axis autorange not working as expected ?

LoganWlv commented 4 years ago

I have a bart chart with X axis autorange enabled. X is of type "category" with numbers and a single string to represent not known values. Somehow the chart is not fully displayed.

Here a minimal example with the issue :

https://codepen.io/loganwlv/pen/mdEdYLW

EDIT: Some extra information can be found there -> stack_overflow It seems with 7.5  instead of 7.5on X the graph is displayed as expected

alexcjohnson commented 4 years ago

Thanks for the report @LoganWlv - this is indeed a bug, the problem is that the range of a category axis is normally expressed in units of the category serial numbers, and as you have 8 categories (0 to 7) and you want to see the whole of each bar (±0.5) the autorange value is [-0.5, 7.5].

The bug is that apparently when we then re-interpret that range to draw the graph, because you have a category of the same name we take 7.5 to mean "the serial number of category 7.5." I imagine we're trying to be flexible and allow you to specify like range: ['apples', 'oranges'] but honestly that's rarely useful because of the ±0.5, or related padding on scatter and other trace types.

LoganWlv commented 4 years ago

@alexcjohnson Thanks for the details.

So you confirm that it should be fixed ? If yes, do you think you could tell me in which file to search for this logic so I may be able to do the fix ?

alexcjohnson commented 4 years ago

The issue is in converting from "range values" to "calcdata" or "linearized" values, ie the functions ax.r2c or ax.r2l here:

https://github.com/plotly/plotly.js/blob/8b76d46c058dedea34bd693daf962e4b8f1135eb/src/plots/cartesian/set_convert.js#L318-L324

both of which call out to getCategoryPosition:

https://github.com/plotly/plotly.js/blob/8b76d46c058dedea34bd693daf962e4b8f1135eb/src/plots/cartesian/set_convert.js#L180-L186

I don't think we want to muck with getCategoryPosition, because it's also used by ax.d2r and ax.d2l_noadd and the "d" ("data") values are expected to be category names. Also "range values" are also used by annotations, so in case anyone depends on being able to position an annotation based on the category name I don't think we want to completely drop this ability. So I think we need a new function here that just swaps the precedence, something like:

function getRangePosition(v) {
    return isNumeric(v) ? +v : getCategoryIndex(v);
}

In order to complete the fix, in addition to ensuring this doesn't break any existing tests we'll need a few new ones:

Test the case you care about - can be exactly the graph in your codepen, just draw it and check that gd._fullLayout.xaxis._rl is [-0.5, 7.5] rather than [-0.5, 4] - This test should looks similar to (and can be placed next to):

https://github.com/plotly/plotly.js/blob/8b76d46c058dedea34bd693daf962e4b8f1135eb/test/jasmine/tests/axes_test.js#L1658

Test that if you add an annotation to a graph that has some numeric categories and some non-numeric categories, providing a non-numeric category as the annotation position will attach it to that category, but providing a numeric category will attach it to the serial number position instead. There actually is a basic test of the latter case:

https://github.com/plotly/plotly.js/blob/8b76d46c058dedea34bd693daf962e4b8f1135eb/test/jasmine/tests/annotations_test.js#L1000

But that's in the context of testing clicktoshow and doesn't really test where it's positioned. For that purpose we may need to test the bounding box, though those tests are a bit tricky to get working well across platforms. Alternatively we could add a new image test covering these cases - similar to simple_annotation but with a category axis and a few more annotations.

LoganWlv commented 4 years ago

Thanks a lot. I'll try to make the fix

LoganWlv commented 4 years ago

@alexcjohnson I created a PR. As you said I did add a unit test to check the computed range + one image test, it is working.

Though in local when running jasmine tests it breaks on multiple tests (with and without the fix). So I am wondering if there is something wrong in my local configuration and if I could mimic the circleCI test pipeline with docker.

Some tests which are broken in local (without the fix)

Chrome 86.0.4240.75 (Mac OS 10.15.7) Animating multiple axes @flaky updates ranges of secondary axes (date + category case) FAILED  Chrome 86.0.4240.75 (Mac OS 10.15.7) axis zoom/pan and main plot zoom updates matching axes no-constrained x-axes matching x-axes subplot case zoombox on xy FAILED
    Error: Expected 1.4770202987361165,2.33875909613175 to be close to 1.494,2.35 xaxis - after zoombox on xy |input

  Chrome 86.0.4240.75 (Mac OS 10.15.7) axis zoom/pan and main plot zoom updates matching axes no-constrained x-axes matching x-axes subplot case drag e on xy FAILED
    Error: Expected -0.2464572960551513,1.3391504565336907 to be close to -0.245,1.366 xaxis - after drag e on xy |input

Chrome 86.0.4240.75 (Mac OS 10.15.7) Indicator plot numbers scale down but never back up if domain size is constant FAILED
    Error: Expected '0.6740981304309663' to be close to 0.8, 1, 'should scale down'.

...

alexcjohnson commented 4 years ago

Great, I'll take a look at your PR!

Don't worry about the local test failures - some of those interaction tests are tricky to get working right, and the @flaky ones sometimes fail anyway.

alexcjohnson commented 4 years ago

Closed by @LoganWlv in #5211

plotly / plotly.js

Bar chart X axis autorange not working as expected ? #5200