chartjs / chartjs/chartjs-chart-financial

BUG on calculating the min sample size

Open
#134 0 comments 1 reaction 0 assignees View on GitHub
Dominant language
JavaScript
Stars
809
Forks
199
PR merge metrics
No merged PRs in 30d

Description

Hi there,
I am using chartjs v3.6.0 and moment (with [moment adapter](https://github.com/chartjs/chartjs-adapter-moment)) 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;

```javascript
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);

```javascript
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.

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.