Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow configurable adder/subtractor for x/y axis padding. #892

Closed
wants to merge 1 commit into from

Conversation

mtraynham
Copy link
Contributor

Currently, we use dc.utils.add / dc.utils.subtract for adding/subtracting padding from the min/max of a scales domain. dc.utils.add/subtract is nice because it handles quite a few cases, but it misses on out on floats and date types other than date.

I had considered using the same function for adding/subtracting with a negated padding value, but this seems a bit more configurable and it retains the usage of dc.utils.add/subtract.

@gordonwoodhull gordonwoodhull added this to the v2.0 milestone May 24, 2015
@gordonwoodhull
Copy link
Contributor

I've been sitting on the fence about this one for a long time because it seems excessively complicated.

@mtraynham, if you're still around, couldn't we just fix dc.utils.add / dc.tools.subtract instead?

@mtraynham
Copy link
Contributor Author

mtraynham commented Nov 27, 2016

This is pretty old, so I'm trying to refresh my memory. The fix was to provide a means of normalizing the padded number for all types of scales, allowing users to provide their own interpretation of what it means to "add 1" to the right and "subtract 1" from the left.

Take dc.utils.add for instance.

  1. For Date handling, adding 1 always means add a full day. There may be cases where x scales are ranged by an interval other than day, such as seconds, minutes, hours, weeks, months, years. A padding of 1, will add a full day to the left and right of date scale, even if the scales full domain is a 5 minute interval. This would cause padding to unproportionally shrink the view of data.
    In my case, I used moment.js to add/subtract the padding (post this patch).
subtract: function (d, x) {
    return d instanceof Date ? moment(d.getTime()).subtract(x, 'hours').toDate() : d - x;
},
add: function (d, x) {
    return d instanceof Date ? moment(d.getTime()).add(x, 'hours').toDate() : d + x;
}

This may actually have a resolution other than my own. If the dc.utils.add function somehow took in consideration the xUnits property on the coordinate chart (such as d3.interval.minutes), that could help infer what adding 1 really means.

  1. The other weird case, is the inconsistency in padding with percentages. Say the following conditions apply:
chart.x(d3.scale.linear().domain([0, 1])).xAxisPadding('0.5%');

_chart.xAxisMin & _chart.xAxisMax will actually generate different paddings based on the min and max of the data. In the min case (0) it still generates 0, and in the max case (1) it generates 1.5.
To circumvent this issue, I felt that padding should also have knowledge of the domain of data and instead padding should be generated from the extent of the domain to provide equal values for left and right.

// (1 - 0) * 0.5
var domain = 1;
subtract: function (d, x) {
    return d instanceof String ? d - dc.utils.subtract(domain, x) : dc.utils.subtract(d, x);
},
add: function (d, x) {
    return d instanceof String ? d + dc.utils.add(domain, x) : dc.utils.add(d, x);
}

The solution allowed me to fix both problems simultaneously, even if they are wildly different. I felt that dc.utils.add/dc.utils.subtract might often need more scope of the data than generically adding a numerical value.

@gordonwoodhull
Copy link
Contributor

I see. Woof, that gets complicated pretty quick. I guess we have no choice but to push the complexity onto the user.

Okay, I'll go with this. Thanks for the rationale - I'll add some of that to the docs.

@mtraynham
Copy link
Contributor Author

mtraynham commented Nov 27, 2016

Well, can we take a step back before you go ahead. I'm not against other ideas here and this is rather complex. If you think documentation can convey this fluently, I'm all for the patch, but I'd probably sacrifice functionality for the sake of understanding how this works and why it works this way.

These were the only two cases that I came across, and it only came into issue with the coordinate grid charts. Because these were the only 2 cases, if we could figure out how to make date addition work for all types of xUnits and potentially take the domain extent in consideration for percentages, I'm all for that if it simplifies the API/documentation.

@stillesjo
Copy link
Contributor

stillesjo commented Nov 28, 2016

Hi 👋
I took a shot at this in a fork that we use since we wanted to have padding in hours instead of days, and to be able to customize it to the data the chart visualizes.

My approach was to update the subtract and add functions to utilize the d3.time.[day/hour/e t c].offset function to update the date instead of just adding it by changing the date.

So, instead of this:

var d = new Date();
d.setTime(l.getTime());
d.setDate(l.getDate() + r);
return d;

You get this:

t = t || 'day';
return d3.time[t].offset(l, r);

t defaults to 'day' but can be any of the time intervals supported by d3.

A PR for this can be seen here (in a fork). I was going to create a PR for this on dc-js/dc.js but found this conversation (through a colleague) and thought I'd discuss it first.

This solution doesn't solve the second problem mentioned by @mtraynham.

@mtraynham
Copy link
Contributor Author

mtraynham commented Nov 30, 2016

@stillesjo Oh that's awesome! Maybe I can stop using moment.

So the coordinate-grid chart currently has xUnits which could potentially be reused for the purpose this. xUnits supports all the d3.time[t].range functions (i.e. d3.time.minutes, d3.time.hours), which are aliases to the range functions of intervals (i.e. d3.time.minute.range, d3.time.hour.range).

Maybe instead, we have xUnits just take the top level interval functions (i.e. d3.time.minute, d3.time.hour). It's typically the same thing that is passed in for scales. For brushing, we'd call the range function where appropriate and for padding we would call the offset function.

To support that, we'd have to update the dc xUnit variants for integer/float/ordinal. Maybe something like:

dc.units.integers = {
    range: function (start, end, step) {
        if (!step) {
            step = 1;
        }
        return Math.abs((end - start) / step);
    },
    offset: function (num, step) {
        return num + step;
    }
};

dc.units.ordinal = function (domain) {
    return {
        range: function (start, end, step) {
            return domain;
        },
        offset: function (value, step) {
            return value;
        }
    };
};

dc.units.float = function (precision) {
    return {
        range: function (start, end, step) {
            if (!step) {
                step = 1;
            }
            var d = Math.abs((end - start) / (precision * step));
            if (dc.utils.isNegligible(d - Math.floor(d))) {
                return Math.floor(d);
            } else {
                return Math.ceil(d);
            }
        },
        offset: function (num, step) {
            return num + precision * step;
        }
    };
};

We then could reuse all these functions for padding, although, we may need to support yUnits as well. As you said, this doesn't address the percentage padding problem, but these solutions don't necessarily have to be done together.

@gordonwoodhull
Copy link
Contributor

I like the @stillesjo solution. It is cleverly backward-compatible and yet pretty simple and easy to understand. I think I'll merge that for 2.0 if there are no objections.

I also noticed the relation to xUnits and it would be awesome to specify all units in one place. It looks like the @mtraynham solution will be forward-compatible and we can introduce it in 2.1, since d3.time hasn't changed much in 4.0.

I bet we can also make it backward-compatible by testing if xUnits() is a function or an object. If you pass a function, you won't get the automatic offset stuff.

@gordonwoodhull
Copy link
Contributor

Note the @stillesjo implementation is not forward-compatible because the intervals are directly in the d3 namespace (d3.timeWeek etc.). I don't think it would be a big deal to map the names, though.

@gordonwoodhull
Copy link
Contributor

Okay I'm merging the @stillesjo solution to close this, and opened #1230 to track the more general idea for 2.1

gordonwoodhull pushed a commit that referenced this pull request Dec 2, 2016
Add method for xAxisPaddingUnit

Fix lint warnings

Add more test cases

Refactor subtract/add util functions

closes #892
gordonwoodhull added a commit that referenced this pull request Apr 19, 2018
gordonwoodhull added a commit that referenced this pull request Apr 19, 2018
@gordonwoodhull
Copy link
Contributor

As of 3.0 alpha 11, the xAxisPaddingUnit is preferred to be a d3 time interval, not the name of it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants