Skip to content

Conversation

ktsaou
Copy link
Contributor

@ktsaou ktsaou commented Feb 10, 2018

As described in #781, the library makes wrong decisions when the y-range is zero.
In such cases (y0 === y1), this PR changes:

y0 -= 0.5
y1 += 0.5

so that the chart is properly rendered.

@coveralls
Copy link

coveralls commented Feb 10, 2018

Coverage Status

Coverage decreased (-0.1%) to 90.096% when pulling b5b0d36 on ktsaou:master into 6611837 on danvk:master.

Copy link
Owner

@danvk danvk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! This will be good to merge if you make the requested change for log scales and add a unit test.

src/dygraph.js Outdated

// special case #781: if we have no sense of scale, center on the sole value.
if (y0 === y1) {
y0 -= 0.5;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For a log scale, this could drop y below zero, which will produce NaNs. In the logscale branch of the conditional on line 2546, you can halve y0 and double y1 instead.

@ktsaou
Copy link
Contributor Author

ktsaou commented Feb 10, 2018

There are 3 cases where y0 === y1:

  1. they are both zero
  2. they are both positive
  3. they are both negative

I think this will properly handle all cases, for both normal and logscale rendering:

      if (y0 === y1) {
      	if(y0 === 0) {
      	  y1 = 1;
      	} else {
      	  var delta = Math.abs(y0 / 2);
          y0 -= delta;
          y1 += delta;
      	}
      }

So, only when it is zero, set y1 = 1 (from the charts I render, I see this must be a default for other cases too - for example when stacked charts have only zeros, the library already visualizes 0 to 1).

Otherwise provide a range of -1/2 to +1/2 of what already the number is. So, do not change the sign. If it is negative keep it negative, if it is positive keep it positive.

Do you agree?

Of course I agree there should be a unit test for this part of the code - its absence is what caused the bug in the first place. However, I need some guidance - I don't have a clue how your unit testing works, or how to configure one.

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

Successfully merging this pull request may close these issues.

4 participants