Y Axes alignment option #985

gazal-k · 2015-08-13T04:39:51Z

When there are two y axes and at least one of them has values in the negative range, the 0 level on both axes don't line up.
Having an option to line them up seemed like a nice to have in my opinion.

Example:

Without Setting alignAxes option:

Setting alignAxes option:

chart.alignAxes(true);

Pending

tests
docs (couldn't find how to generate the docs)

gordonwoodhull · 2015-09-14T22:11:51Z

I agree this is a good feature. However, I find the implementation excessively complicated.

I think you can implement this with much simpler, much less code by adapting the two places where yAxisMin, rightYAxisMin, yAxisMax, rightYAxisMax are used, rather than changing how they are defined.
You have made yAxisMin and rightYAxisMin mutually recursive, each one calling the other with the alignment flag implicitly false. This is inefficient, because all the data will be evaluated twice. I also find this implementation confusing.
At least to me, it seems more intuitive to leave the meaning of these functions alone, to let them still return the actual minimum and maximum for each axis. Then, after each has been calculated, adjust all four values and set the ranges for both left and right axes. Of course this means combining prepareLeftYAxis and prepareRightYAxis, which is fine, because they are only ever called at the same time, from _prepareYAxis. The combined function could be prepareYAxes(left, right), where left and right are boolean parameters.

Also, this definitely needs tests in spec/composite-chart-spec.js, showing that the axes are not aligned with alignedAxes false, and are aligned with alignedAxes true. IMO the flag should also be called alignYAxes.

Conflicts: dc.js dc.js.map dc.min.js dc.min.js.map src/composite-chart.js

as per review comment from gordonwoodhull

gazal-k · 2015-09-15T15:24:40Z

@gordonwoodhull sorry about that. I'd been using the composite chart in an application we're developing. Originally this axes alignment feature was developed outside composite-chart's source which actually was more optimal. I kinda messed it up when I tried to put that into composite-chart code.
So, now I've used the original implementation and introduced it into composite-chart as per your recommendations.
Just a minor difference though, because of cyclomatic complexity, I'd to refactor it again. There's no unified prepareYAxes function. Instead the calculation for axes alignment is done in a separate function and its result fed to prepareLeftYAxis and prepareRightYAxis.

I haven't added tests in spec/composite-chart-spec.js yet. Before that, please let me know if the refactor is ok.

gordonwoodhull · 2015-10-06T15:41:08Z

dc.js

-        if (leftYAxisChildren().length !== 0) { prepareLeftYAxis(); }
-        if (rightYAxisChildren().length !== 0) { prepareRightYAxis(); }
+        var left = (leftYAxisChildren().length !== 0), right = (rightYAxisChildren().length !== 0),
+        ranges = calculateYAxesRanges(left, right);


Needs var otherwise ranges will end up in the global scope.

@gordonwoodhull looks like I mislead you with the indentation. I'll just change the multivar declare for better readability.

gordonwoodhull · 2015-10-06T15:45:03Z

Ah, this looks much more clear! And now I understand why you took the prior approach, modifying these classes from outside is indeed tricky. I like that the new approach doesn't change existing interfaces.

This can go into the 2.0 branch, once you add tests. Thanks!

Conflicts: dc.js.map dc.min.js dc.min.js.map web/js/dc.js.map web/js/dc.min.js web/js/dc.min.js.map

gordonwoodhull · 2015-10-07T12:10:41Z

Duh, right, fooled by the indent. Thanks!

gordonwoodhull · 2015-10-31T03:14:23Z

Hi @gazal-k.

Thanks again! I'm currently merging this. The design and code structure is very good, but I'm not sure about the actual calculation. It can cause one of the axes actually to flip over, in the simple Jasmine test case I built where one of the charts is all negative.

Could you explain the rationale behind taking the ratio of minimum and maximum on one axis, and then applying this ratio to the other axis? It seems like it does cause the zero lines to match up, but might not always keep both plots with the chart boundaries or properly oriented.

add tests force resize when changing alignYAxes rename calculateYAxisRanges add author

gordonwoodhull · 2015-10-31T08:13:12Z

Merged in 2.0.0 beta 20

gazal-k · 2015-10-31T12:24:40Z

@gordonwoodhull sorry for the delayed reply. I don't remember exactly how I applied the math for it. Let me try the test you mentioned.

gazal-k · 2015-10-31T14:20:27Z

@gordonwoodhull thanks for that. I had totally missed that case. And sorry that I made you write the tests 😢
#1033 should fix the issue. Please check and let me know

gordonwoodhull · 2015-12-04T17:21:09Z

Note: I think it was implied that elasticY must be true to enable this feature. In the first pass we didn't account for this, but that caused breakage in #1056. I'm enforcing that now.

In other words, you can't manually apply the Y domains and use this feature, because this feature expects to set the Y domains for you.

for #985

gazal-k · 2015-12-05T18:07:06Z

@gordonwoodhull sorry about that. so alignYAxes should have no effect when elasticY is false, correct?

Shall make the change here: #1033

gordonwoodhull · 2015-12-05T18:24:23Z

You agree this is correct, right? I think I have fixed it in the commit referenced above.

It may have been me who broke this in the first place, since I made some additional changes after testing your code.

Please review the fix and then document this requirement in #1033. Thanks!

gazal-k · 2015-12-05T18:36:25Z

Hadn't noticed 3d13e68.
But that only changes the spec, right? I didn't see any change in https://github.com/dc-js/dc.js/blob/develop/src/composite-chart.js that limits alignYAxes from being applied when elasticY is false.

And yes, I do agree that alignYAxes need only be applied when elasticY is true.

gordonwoodhull · 2015-12-05T19:22:22Z

Whoops I linked one commit but not the other. Here is the fix: d1c087d

gazal-k added 2 commits August 12, 2015 15:02

adding option to composite-chart to align 0 level across both y axes

c38cfff

jscs errors fixed

49628fd

gazal-k added 5 commits September 15, 2015 16:51

Merge branch 'develop' into composite-chart-enh

0876635

Conflicts: dc.js dc.js.map dc.min.js dc.min.js.map src/composite-chart.js

rename alignAxes to alignYAxes

68b1786

combining prepareRightYAxis and prepareLeftYAxis into prepareYAxes

ec8f2e2

as per review comment from gordonwoodhull

refactoring to reduce cyclomatic complexity

d3e15f9

migration to jsdoc for changes in composite-chart

b34289e

gordonwoodhull reviewed Oct 6, 2015
View reviewed changes

gordonwoodhull added the enhancement label Oct 6, 2015

gordonwoodhull added this to the v2.0 milestone Oct 6, 2015

gazal-k added 2 commits October 7, 2015 12:13

splitting multivar declare

4079b4c

Merge branch 'develop' into composite-chart-enh

48bbd45

Conflicts: dc.js.map dc.min.js dc.min.js.map web/js/dc.js.map web/js/dc.min.js web/js/dc.min.js.map

gordonwoodhull added a commit that referenced this pull request Oct 31, 2015

cleanup of #985

2dd561f

add tests force resize when changing alignYAxes rename calculateYAxisRanges add author

gordonwoodhull closed this Oct 31, 2015

gazal-k mentioned this pull request Oct 31, 2015

Y Axes alignment option: addendum #1033

Closed

gazal-k deleted the composite-chart-enh branch October 31, 2015 14:20

gordonwoodhull added a commit that referenced this pull request Dec 4, 2015

alignYAxes should not be applied without elasticY

3d13e68

for #985

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Y Axes alignment option #985

Y Axes alignment option #985

gazal-k commented Aug 13, 2015

gordonwoodhull commented Sep 14, 2015

gazal-k commented Sep 15, 2015

gordonwoodhull Oct 6, 2015

gazal-k Oct 7, 2015

gordonwoodhull commented Oct 6, 2015

gordonwoodhull commented Oct 7, 2015

gordonwoodhull commented Oct 31, 2015

gordonwoodhull commented Oct 31, 2015

gazal-k commented Oct 31, 2015

gazal-k commented Oct 31, 2015

gordonwoodhull commented Dec 4, 2015

gazal-k commented Dec 5, 2015

gordonwoodhull commented Dec 5, 2015

gazal-k commented Dec 5, 2015

gordonwoodhull commented Dec 5, 2015

Y Axes alignment option #985

Y Axes alignment option #985

Conversation

gazal-k commented Aug 13, 2015

Example:

Pending

gordonwoodhull commented Sep 14, 2015

gazal-k commented Sep 15, 2015

gordonwoodhull Oct 6, 2015

Choose a reason for hiding this comment

gazal-k Oct 7, 2015

Choose a reason for hiding this comment

gordonwoodhull commented Oct 6, 2015

gordonwoodhull commented Oct 7, 2015

gordonwoodhull commented Oct 31, 2015

gordonwoodhull commented Oct 31, 2015

gazal-k commented Oct 31, 2015

gazal-k commented Oct 31, 2015

gordonwoodhull commented Dec 4, 2015

gazal-k commented Dec 5, 2015

gordonwoodhull commented Dec 5, 2015

gazal-k commented Dec 5, 2015

gordonwoodhull commented Dec 5, 2015