novus / nvd3

A reusable charting library written in d3.js
http://nvd3.org/
Other
7.22k stars 2.15k forks source link

y axis going crazy on first value of 10+ #1918

Open erik777 opened 7 years ago

erik777 commented 7 years ago

I am using multiChart with line to chart NUGT close prices. For some reason it can chart the past 90 days without problem until it gets to 1/5. If I chart until 1/4, it looks great.

I narrowed it down by copy the data into the code. It simply chokes on the first row it gets where the price is $10 or greater! If I make it 9.9999, it is happy.

When it is working, it makes the max value of the y axis 10. And, when it breaks, it turns the y axis upside down, from a low value to high top down, instead of high to low, and makes it a tiny subset, which explains why the line is missing for most X values and vertical for some. So, this issue seems to have something to do with how it calculates the y scale. But, why would the first value over 10 be such an issue?

When I use other symbols, such as GLD or $SPX.X, it charts without a problem. It is very baffling that it doesn't like the prices of NUGT.

To give you an example, I took the line that it choked on (first ilne) and created two variations of it, leaving only one uncommented. In the 2nd line, I changed close price to 10. It still chocked on it. But, third line does not choke. I can make it 9.99, or even 9.9999, and it is fine. Touch to or above, and the y Axis becomes a mess.
{"High":"10.2400","Low":"9.9300","Volume":"109998.3516","TimeInMillis":"1483630200000","Close":"10.1700","Open":"9.9600"} {"High":"10.2400","Low":"9.9300","Volume":"109998.3516","TimeInMillis":"1483630200000","Close":"10","Open":"9.9600"}
{"High":"10.2400","Low":"9.9300","Volume":"109998.3516","TimeInMillis":"1483630200000","Close":"9.9999","Open":"9.9600"}

I don't see the ability to attach to an issue, or I'd share screenshots and the raw data.

liquidpele commented 7 years ago

Please create a jsfiddle demonstrating the issue. When doing so, please link to the latest build files in the master branch by utilizing rawgit.com. This really helps to speed up debugging on our end, thanks!

erik777 commented 7 years ago

How about a plunk?

http://plnkr.co/edit/N4kGrsIfHl95gFk89NzC?p=preview

erik777 commented 7 years ago

I was also able to reproduce with UVXY which also crossed the $10 threshold. Many stocks I tried so far had no issue, but stayed above or below $10. Did a scan to find stocks that crossed $10, and sure enough, they produce this issue: OR, NBCT, EDUC. Time frame I'm charting when doing this is 8/1/2016 to 1/4/2017, close on 30m intervals.

liquidpele commented 7 years ago

Well, that plunk is using angular2, any examples need to be pure-javascript to avoid us having to debug issues with other frameworks or wrappers. This appears to be using the ng2-nvd3 project. If you can't reproduce the issue with pure javascript and nvd3 then you'll want to open an issue with that project.

erik777 commented 7 years ago

Yeah, that's not possible as that developer has been unreachable for past 6 months, and I only learned how to use nvd3 with ng2 and Typescript.

I figured though that someone who is good at Javascript nvd3 would have no trouble re-creating from that plunker as all it does is set options on a chart and provide data.

The ng2 wrapper just creates an ng2 interface to nvd3. It doesn't try to do anything with the data, or try to add features. It does not inspect the data or try to play with the y axis. The actual ng2-nvd3.ts is only 220 lines, including blanks.

https://github.com/krispo/ng2-nvd3/blob/master/lib/ng2-nvd3.ts

liquidpele commented 7 years ago

I'm sorry but I'm basically one guy here maintaining this on the side in my spare time, I honestly dont have the time to try and reproduce issues, especially when many of them turn out to be misunderstandings or problems with other projects. I hope you can understand.. or, if you want to just try and fix the issue and send me a pull request I can look at that :)

erik777 commented 7 years ago

This bug is a serious show stopper. It's not like legends are positioned incorrectly, or colors are wrong. This is an inability to chart any equities that cross the $10 price.

I was able to copy the nvd3/examples/multiChart.html and produce the bug with that. This rules out ng2 and narrows it to an nvd3 issue.

Am trying to get that to work in a plunker, but the page is just blank for some reason. I'll update if I can get the plunker to work.

erik777 commented 7 years ago

There ya go:

http://plnkr.co/edit/sJADUjoVfMrWarLso1uU?p=preview

To play with it, look for this section:

     // Only uncomment one of the following 3 lines
     // to test different close prices.
     // The third one works because Close is below
     // 10.  
        {"High":"10.2400","Low":"9.9300","Volume":"109998.3516","TimeInMillis":"1483630200000","Close":"10.1700","Open":"9.9600"}

// { "High": "10.2400", "Low": "9.9300", "Volume": "109998.3516", "TimeInMillis": "1483630200000", "Close": "10", "Open": "9.9600" } // { "High": "10.2400", "Low": "9.9300", "Volume": "109998.3516", "TimeInMillis": "1483630200000", "Close": "9.9999", "Open": "9.9600" }

You'll see that simply changing the Close price to a number under 10 causes the chart to display correctly.

erik777 commented 7 years ago

You'll be happy to know I figured out the cause of the bug (conceptually ... not in code terms, as I still do not know nvd3 code.)

The values are strings in the JSON. In the plotting, nvd3 happily converts them to numbers. But, when determining the y scale, it must be computing max and min on the strings. I discovered this when I was using d3.max to work around this problem by setting yDomain1. I had to convert the values d3.max used to numbers in order to get the correct value. That's when I realized that is the issue nvd3 has. This explains why 9.9999 worked, but 10 introduced the issue, because the string "9.9999" is greater than "10". It is even greater than "89".

If you find max calculations in the y scaling code, you'll find the location of the bug and be able to easily fix. It might even be using d3.max.

erik777 commented 7 years ago

Here's my line for calculating it in correctly on the Close price in Typescript:

        var maxY = d3.max(data[0].values, function(d) {return +d["Close"];});

Note that the '+' is what converts the string to a number. Without that, like nvd3, it gave the incorrect result. The '+' must work in Javascript because I just looked at the .js file it generates and this line is untouched. So, you may just need to prefix a '+' before the variable used to calculate the min/max.

liquidpele commented 7 years ago

ah... yea, using strings instead of numbers I'm sure would cause a lot of weirdness...

erik777 commented 7 years ago

The tooltips were fixed too when I changed JSON to numbers, which use the TimeInMillis field for X axis.

I'd be half tempted to fix nvd3 myself, but I'm not setup to develop and build it. Plus, I looked at the code, and it's pretty complicated. I don't know what it's trying to do half the time. There aren't as many comments as I'd like in the multiChart model code. Plus, I don't know D3.

There are a lot of functional improvements I'd like to see. The big one is I'd like to add the candlestickChart to the multiChart because I'm displaying stock prices (candlestick) along with studies like moving averages (line). I also need to add horizontal lines for fibonacci studies. That's where I might start to play with D3 directly, since I'm not aware of that ability in nvd3 currently.

I'm considering learning a bit about D3 and playing with overlaying the charts to enhance.

Any tips on getting started building and testing nvd3 or using D3 to enhance?

liquidpele commented 7 years ago

It seems to me that this isn't a bug, and that passing strings and expecting them to act like numbers is not a valid use case...

liquidpele commented 7 years ago

Or maybe you just meant in general... to develop, you pretty much just need to do npm install, edit the src files, and then grunt production and it'll build it for ya :)

https://github.com/novus/nvd3#contributing

erik777 commented 7 years ago

Well, defect or not depends on requirements. Since plotting currently converts strings to numbers, would be consistent and nice to have it support strings in y scaling and tooltips, as well; particularly since it is so easy to do.

Thanks for the link! I'll use it to get setup when I'm ready to play with D3 and NVD3. Here's an entertaining demo of D3 where you have lots of stars around your github name when you press the Play button:

http://ghv.artzub.com/#repo=nvd3&climit=100&user=novus

jeznag commented 7 years ago

@erik777 why don't you just do:

chart
  .y((d, i) => +d['Close'])

I agree with liquidpele, you shouldn't be passing in strings into a chart.