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

Scale using data with value/key is broken for time series #44

Open
tpitale opened this issue Feb 11, 2016 · 4 comments
Open

Scale using data with value/key is broken for time series #44

tpitale opened this issue Feb 11, 2016 · 4 comments

Comments

@tpitale
Copy link

tpitale commented Feb 11, 2016

else {
var min_value = min(data, getValue);
domain = [
min_value < 0 ? min_value : 0,
max(data, getValue)
];
}

Does not work for time (and probably a lot of other scale types). The first time is always going to be > 0. But, we should still use the min value from the series.

@timhall
Copy link
Contributor

timhall commented Feb 11, 2016

Thanks @tpitale for bringing this up, it was a fairly large assumption to default to 0 as the minimum for all non-ordinal scales. I'll update it to simply use the minimum shortly (with the existing option to override with domain, if specified).

@tpitale
Copy link
Author

tpitale commented Feb 11, 2016

I'd be happy to submit a PR if you'd like one.

@tpitale
Copy link
Author

tpitale commented Feb 11, 2016

Or I could wait to try until after #43 is completed/merged.

@timhall
Copy link
Contributor

timhall commented Feb 11, 2016

I'm a little on the fence about the future of createScale currently, it does too much and makes a few too many assumptions for my tastes. I have been thinking of breaking it into a few different approaches:

  • Add scaleBandSeries that extends scaleBand in d3 v4. This covers the adjacent, centered, and series options
  • Add getMinMaxDomain and getOrdinalDomain that handle data and key options for setting domains for linear-link and ordinal scales, respectively.

The getMinMaxDomain should cover this case, although I just looked at it and it has the same issue: vNext:src/mixins/xy (which in some ways is more egregious here since it has min in the name). I'll update it in that PR before it gets merged (which should be today or tomorrow).

I think the new approach is much more direct (with the tradeoff being slightly more to type), although I'm open to adding createScale back as an add-on.

// Before
var xScale = {type: 'time', data: data, key: 'date'};
var x2Scale = {type: 'ordinal', domain: ['a', 'b', 'c'], adjacent: true, series: 2};

// After
var xScale = d3.time.scale()
  .domain(getMinMaxDomain(data, 'date'));
var x2Scale = scaleBandSeries()
  .domain(['a', 'b', 'c']).adjacent(true).seriesCount(2);

I'm glad to hear you're using the library and would love to have your feedback!

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

No branches or pull requests

2 participants