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

Can zoom out but cannot zoom in again, also weird volume scaling #59

Closed
char101 opened this issue Sep 13, 2019 · 22 comments
Closed

Can zoom out but cannot zoom in again, also weird volume scaling #59

char101 opened this issue Sep 13, 2019 · 22 comments
Labels
improvement Something might work better

Comments

@char101
Copy link

char101 commented Sep 13, 2019

Hi,

I found this particular case with a stock data. Other stock data is working fine.

The weird case: no x axis labels, wrong zooming (compare with the chart below with the same zoom level, and also not uniform volume bar widths), can be zoomed in but cannot be zoomed out. I don't see any error message in the dev console.
2019-09-13 15_08_57-chart

The normal case
2019-09-13 15_09_13-chart

Dataset
ABDA.zip

@C451
Copy link
Collaborator

C451 commented Sep 13, 2019

That's interesting. First thought: maybe the timeframe was detected incorrectly. https://github.com/C451/trading-vue-js/blob/master/src/stuff/utils.js#L96

@char101
Copy link
Author

char101 commented Sep 14, 2019

You are right, this is the output of console.log('detect_interval', r, i1, i2)

detect_interval 86400000 86400000 86400000 2 trading-vue.js:5286
detect_interval 432000000 432000000 1468800000 trading-vue.js:5286
detect_interval 86400000 172800000 86400000 trading-vue.js:5286

So detect_interval only looks at the first and last time gap. I think for correctness it needs to find the minimum of all the time gaps.

I changed the implementation into

detect_interval(ohlcv) {
        let min = 0
        if (ohlcv.length > 1) {
            for (let i = 1, n = ohlcv.length; i < n; ++i) {
                const gap = ohlcv[i][0] - ohlcv[i-1][0]
                if (!min || gap < min) {
                    min = gap
                }
            }
        }
        return min
    },

But although now it returns 86400000 like the other chart, I still couldn't able to zoom out after zooming in. Also zooming in feels very slow.

@C451
Copy link
Collaborator

C451 commented Sep 14, 2019

detect_interval should be O(1) for the current implementation. Called every time the layout is updated. One idea is to add forcedInterval prop, because otherwise it works in 99% of cases.

I remember "zoom out" issue, it caused by the minimum candle requirement somewhere.

@char101
Copy link
Author

char101 commented Sep 14, 2019

Since it only depends on the time data, can't it be cached?

@C451
Copy link
Collaborator

C451 commented Sep 14, 2019

The question is : "When to uncache?". Because the data update will trigger watch() in both cases: when changing the entire data set or only the last candle.

So, I'd prefer to use Occam's razor here. If you can suggest more elegant solution, let's discuss.

https://github.com/C451/trading-vue-js/blob/master/src/components/Chart.vue#L263

@char101
Copy link
Author

char101 commented Sep 14, 2019

It isn't Occam razor though because the output of the two functions is different, that is on the border case I described in this issue.

As for when to update the cache value, can't it store a reference to the ohlcv data and later compare it by identity. If it is the same then it means any change is streaming data and the interval should be the same and doesn't need to be recomputed. If it is different then it means that the data is fresh.

@C451
Copy link
Collaborator

C451 commented Sep 14, 2019

Maybe not a canonical razor, but In terms of multiplying lines of code, most certainly it is :)

Yes, it's possible to identify the change. I'm thinking about if old_length !== new_length && old_first_interval !== new_first_interval . It should work in 99.9% of cases. Your idea?

@char101
Copy link
Author

char101 commented Sep 14, 2019

Programming is a trade-off. Since I don't use streaming data my O(n) implementation above is enough for me. So I just want to clarify that I'm in no way pushing for change in the current implementation.

Yes, it's possible to identify the change. I'm thinking about if old_length !== new_length && old_first_interval !== new_first_interval . It should work in 99.9% of cases. Your idea?

Since detect_interval is a utility function you would still need to store that values somewhere which is one of the factors that increase the line count I think.

A dead-simple approach is probably to cache the value in the array itself. Maybe something like this. I haven't tried it though since I don't use streaming data.

    detect_interval(ohlcv) {
        let min = 0;
        
        // calculate minimum interval or update cached value with streaming data
        if (ohlcv.length > 1) {
            let firstIndex = 1;

            // retrieve cached value
            if (ohlcv.__interval) {
              min = ohlcv.__interval;
              firstIndex = ohlcv.__lastIndex;
            }
            
            let i = firstIndex;
            for (let n = ohlcv.length; i < n; ++i) {
                const gap = ohlcv[i][0] - ohlcv[i-1][0];
                if (!min || gap < min) {
                    min = gap;
                }
            }
            
            ohlcv.__interval = min;
            ohlcv.__lastIndex = i;
        }
        return min;
    },

@C451
Copy link
Collaborator

C451 commented Sep 14, 2019

Programming is a trade-off.

Yep. Like pretty much everything in life.

I'm glad you found a solution for now. Actually, there is no need to store the value. There is a solution with additional 3 or so lines of code, will implement soon.

@C451 C451 added the improvement Something might work better label Sep 14, 2019
@C451
Copy link
Collaborator

C451 commented Sep 14, 2019

@C451 C451 closed this as completed Sep 14, 2019
@C451 C451 reopened this Sep 14, 2019
@char101
Copy link
Author

char101 commented Sep 15, 2019

Cool!

Based on Performance-Analysis-JS, it seems that for loop is faster though. I think if you are concerned with performance, it is sufficient to process only the first 100 points of data. The minimum time gap should be detectable in at least 100 points of data, probably less. Unless it is a tick data.

As for me, for my usage, I have found a better algorithm, which is O(0) 🚀

    detect_interval(ohlcv) {
        return 86400000
    },

@C451
Copy link
Collaborator

C451 commented Sep 15, 2019

it is sufficient to process only the first 100 points of data

Nice idea!

I have found a better algorithm, which is O(0)

I don't have this luxury, developing the lib for everyone :)

You seem to be good at finding simple solutions, maybe you can help to crack the "dynamic grid" prob? The question is how to make a good-looking x-grid with variable step:

https://github.com/C451/trading-vue-js/blob/master/src/components/js/grid_maker.js#L158

More specifically, yes, we can put grid lines wherever is necessary, but in some cases the gap will be too small and we'll need to recalc the entire grid to make it look decent. IDK how tradingview.com handles this, but I guess it can be done in linear time.

This is the only unresolved problem at the moment. When it resolves, stock charts will finally shine (support gaps via hide-gaps prop for example).

--
The problem actually goes a little deeper, because the lib relies on fast mapping functions:

https://github.com/C451/trading-vue-js/blob/master/src/components/js/layout_fn.js

When time is distorted by gaps, the functions will become more complex and there will be a performance hit. All overlays use these function heavily, so this is a concern.

@char101
Copy link
Author

char101 commented Sep 15, 2019

Well, their lightweight-charts implementation is open source, so you can look at how they compute the ticks. It seems complex. Personally I don't use grid and just like this guy, I found there is no logic to their grid widths 😕. Sorry for the lack of help.

@C451
Copy link
Collaborator

C451 commented Sep 15, 2019

Yeah, had the same though. For me, It's more about the gaps rather than widths. But yeah, ok.

@char101
Copy link
Author

char101 commented Sep 15, 2019

About the gap I think you can just pick a point when the gap between the current point and the previous tick is larger than t_step.

Before:
2019-09-15 16_35_36-chart

After
2019-09-15 16_34_58-chart

            let lastTime = undefined
            for (var i = 0; i < sub.length; i++) {
                let p = sub[i]
                if (p[0] % self.t_step === 0 || (lastTime && (p[0] - lastTime) > self.t_step)) {
                    let x = Math.floor((p[0] - range[0]) * r)
                    self.xs.push([x, p])
                    lastTime = p[0]
                }
            }

@C451
Copy link
Collaborator

C451 commented Sep 15, 2019

That's good for aesthetics, but I'm talking about scrolling through 5m stock chart. Try it, It's a nightmare :)

@char101
Copy link
Author

char101 commented Sep 15, 2019

Sorry, I have only tried daily data. If by gap you mean like the holiday gap, then probably the only practical solution is to change the x scaling into indexed where the x position is determined by the index of the time value in the array instead of the time itself.

@C451
Copy link
Collaborator

C451 commented Sep 20, 2019

Change MIN_ZOOM to a lower value. Will solve the zoom issue.

<trading-vue :chart-config = "{ MIN_ZOOM: 5 }">

@msincenselee
Copy link

msincenselee commented Oct 9, 2019

Hi,C451

https://www.echartsjs.com/en/option.html#xAxis

In order to avoid gap in 'None Continuous time series data', we use 'category' for the xAxis.type

@C451
Copy link
Collaborator

C451 commented Oct 11, 2019

@msincenselee Thanks, will look into.

@RusEu
Copy link

RusEu commented Oct 27, 2019

image

I think this is the same problem.

For cypto seems to work perfect, because there is 24/24 data, but for stocks seems to broke.

I'll try to take a look to see if I can help with the development of this fix.

@C451
Copy link
Collaborator

C451 commented Oct 29, 2019

@RusEu yeah, that would be supa cool. Although there is no easy solution because of this:

The problem actually goes a little deeper, because the lib relies on fast mapping functions:

https://github.com/C451/trading-vue-js/blob/master/src/components/js/layout_fn.js

When time is distorted by gaps, the functions will become more complex and there will be a performance hit. All overlays use these function heavily, so this is a concern.

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

No branches or pull requests

4 participants