chartjs / chartjs-chart-financial

Chart.js module for charting financial securities
MIT License
742 stars 199 forks source link

BUG on calculating the min sample size #134

Open MehGokalp opened 1 year ago

MehGokalp commented 1 year ago

Hi there, I am using chartjs v3.6.0 and moment (with moment adapter) i faced an issue while i am trying to use this plugin. Here is the problem;

Screen Shot 2022-11-14 at 00 26 27

After some investigations i found the problem;

function computeMinSampleSize(scale, pixels) {
        let min = scale._length;
        let prev, curr, i, ilen;

        for (i = 1, ilen = pixels.length; i < ilen; ++i) {
            min = Math.min(min, Math.abs(pixels[i] - pixels[i - 1]));
        }

        for (i = 0, ilen = scale.ticks.length; i < ilen; ++i) {
            curr = scale.getPixelForTick(i);
            min = i > 0 ? Math.min(min, Math.abs(curr - prev)) : min; // here min value is becaming NaN because of curr and prev can be NaN
            prev = curr;
        }

        return min;
    }

My solution was (for very quick fix);

function computeMinSampleSize(scale, pixels) {
        let min = scale._length;
        let prev, curr, i, ilen;

        for (i = 1, ilen = pixels.length; i < ilen; ++i) {
            min = Math.min(min, Math.abs(pixels[i] - pixels[i - 1]));
        }

        for (i = 0, ilen = scale.ticks.length; i < ilen; ++i) {
            curr = scale.getPixelForTick(i);
            min = i > 0 && !isNaN(curr) && !isNaN(prev) ? Math.min(min, Math.abs(curr - prev)) : min; // add check for NaN values
            prev = curr;
        }

        return min;
    }

and the result is;

Screen Shot 2022-11-14 at 01 02 05

but as i see scale.getPixelForTick() always returns NaN and it can be because of wrong date parsing but anyway it should not cause wrong output.

This is all i found in couple of hours please add, share your thoughts and i hope it helps everyone who faced this issue! Thank you.