You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@echarts.apache.org by GitBox <gi...@apache.org> on 2022/01/26 05:21:51 UTC
[GitHub] [echarts] fuchunhui opened a new pull request #16431: fix(line): using sampling with lttb, NaN point should not be filtered.
fuchunhui opened a new pull request #16431:
URL: https://github.com/apache/echarts/pull/16431
…. close #14689
<!-- Please fill in the following information to help us review your PR more efficiently. -->
## Brief Information
This pull request is in the type of:
- [x] bug fixing
- [ ] new feature
- [ ] others
### What does this PR do?
<!-- USE ONE SENTENCE TO DESCRIBE WHAT THIS PR DOES. -->
### Fixed issues
<!--
- #xxxx: ...
-->
## Details
### Before: What was the problem?
<!-- DESCRIBE THE BUG OR REQUIREMENT HERE. -->
<!-- ADD SCREENSHOT HERE IF APPLICABLE. -->
### After: How is it fixed in this PR?
<!-- THE RESULT AFTER FIXING AND A SIMPLE EXPLANATION ABOUT HOW IT IS FIXED. -->
<!-- ADD SCREENSHOT HERE IF APPLICABLE. -->
store NaN point, append first to every bucket, adjust Uint Array length.
## Misc
<!-- ADD RELATED ISSUE ID WHEN APPLICABLE -->
- [ ] The API has been changed (apache/echarts-doc#xxx).
- [ ] This PR depends on ZRender changes (ecomfe/zrender#xxx).
### Related test cases or examples to use the new APIs
N.A.
## Others
### Merging options
- [ ] Please squash the commits into a single one when merging.
### Other information
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] echarts-bot[bot] commented on pull request #16431: fix(line): using sampling with lttb, NaN point should not be filtered.
Posted by GitBox <gi...@apache.org>.
echarts-bot[bot] commented on pull request #16431:
URL: https://github.com/apache/echarts/pull/16431#issuecomment-1043819391
Congratulations! Your PR has been merged. Thanks for your contribution! 👍
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] pissang edited a comment on pull request #16431: fix(line): using sampling with lttb, NaN point should not be filtered.
Posted by GitBox <gi...@apache.org>.
pissang edited a comment on pull request #16431:
URL: https://github.com/apache/echarts/pull/16431#issuecomment-1041016471
Seems when I'm running visual regression test(which has roslution 830x750), the current behavior seems to having unexpected points in the null values.
Also I think we can add the case from https://codepen.io/tmscer/pen/yLgGXbo directly.
| Before | After |
| :------------------------------------: | :----------------------------------: |
|  |  |
<img width="107" alt="image" src="https://user-images.githubusercontent.com/841551/154188257-244611ac-8399-4152-8054-e8a0e70305a5.png">
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] pissang edited a comment on pull request #16431: fix(line): using sampling with lttb, NaN point should not be filtered.
Posted by GitBox <gi...@apache.org>.
pissang edited a comment on pull request #16431:
URL: https://github.com/apache/echarts/pull/16431#issuecomment-1041028558
I'm not sure if the behavior I described above is because we append both the NaN value and picked value. What if we only append the NaN value if there is any NaN value exists in one frame, other wise we append the picked value.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] fuchunhui edited a comment on pull request #16431: fix(line): using sampling with lttb, NaN point should not be filtered.
Posted by GitBox <gi...@apache.org>.
fuchunhui edited a comment on pull request #16431:
URL: https://github.com/apache/echarts/pull/16431#issuecomment-1042867558
```
// set frameSize = 3；
const dimStore = [21, 22, 20, 33, 44, 55];
// IF result is [1, 5]
```
Demo 1：
```
const dimStore = [21, null, 20, 33, 44, 55];
// Right result is [0, 1, 5]
// Before, it would be [1, 0, 5], not right, this made the wrong case as you mentioned
```
@pissang
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] pissang removed a comment on pull request #16431: fix(line): using sampling with lttb, NaN point should not be filtered.
Posted by GitBox <gi...@apache.org>.
pissang removed a comment on pull request #16431:
URL: https://github.com/apache/echarts/pull/16431#issuecomment-1041028558
I'm not sure if the behavior I described above is because we append both the NaN value and picked value. What if we only append the NaN value if there is any NaN value exists in one frame, other wise we append the picked value.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] pissang commented on a change in pull request #16431: fix(line): using sampling with lttb, NaN point should not be filtered.
Posted by GitBox <gi...@apache.org>.
pissang commented on a change in pull request #16431:
URL: https://github.com/apache/echarts/pull/16431#discussion_r806412003
##########
File path: src/data/DataStore.ts
##########
@@ -935,7 +935,7 @@ class DataStore {
let area;
let nextRawIndex;
- const newIndices = new (getIndicesCtor(this._rawCount))(Math.ceil(len / frameSize) + 2);
+ const newIndices = new (getIndicesCtor(this._rawCount))(len);
Review comment:
Using `len` as estimated size is simple and safe, but not with best performance. From current strategy, we can know we may have at most two values in each frame(one for picked value and one for NaN value). And we use `Math.ceil(len / frameSize) + 2` to calculate the possible frames.
So when we having millions of data, following code will be more performant and also robust enough to calculate the possible size ```ts
Math.min((Math.ceil(len / frameSize) + 2) * 2, len)
```
##########
File path: src/data/DataStore.ts
##########
@@ -935,7 +935,7 @@ class DataStore {
let area;
let nextRawIndex;
- const newIndices = new (getIndicesCtor(this._rawCount))(Math.ceil(len / frameSize) + 2);
+ const newIndices = new (getIndicesCtor(this._rawCount))(len);
Review comment:
Using `len` as estimated size is simple and safe, but not with best performance. From current strategy, we can know we may have at most two values in each frame(one for picked value and one for NaN value). And we use `Math.ceil(len / frameSize) + 2` to calculate the possible frames.
So when we having millions of data, following code will be more performant and also robust enough to calculate the possible size
```ts
Math.min((Math.ceil(len / frameSize) + 2) * 2, len)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] pissang edited a comment on pull request #16431: fix(line): using sampling with lttb, NaN point should not be filtered.
Posted by GitBox <gi...@apache.org>.
pissang edited a comment on pull request #16431:
URL: https://github.com/apache/echarts/pull/16431#issuecomment-1041016471
Seems when I'm running visual regression test(which has roslution 830x750), the current behavior seems to having unexpected points in the null values.
Also I think we can add the case from https://codepen.io/tmscer/pen/yLgGXbo directly.
Before:
After:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] pissang edited a comment on pull request #16431: fix(line): using sampling with lttb, NaN point should not be filtered.
Posted by GitBox <gi...@apache.org>.
pissang edited a comment on pull request #16431:
URL: https://github.com/apache/echarts/pull/16431#issuecomment-1041016471
I'm not sure but when I'm running visual regression test(which has roslution 830x750), the current behavior seems to having unexpected points in the null values.
Also I think we can add the case from https://codepen.io/tmscer/pen/yLgGXbo directly.
Before:
After:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] pissang commented on a change in pull request #16431: fix(line): using sampling with lttb, NaN point should not be filtered.
Posted by GitBox <gi...@apache.org>.
pissang commented on a change in pull request #16431:
URL: https://github.com/apache/echarts/pull/16431#discussion_r809644902
##########
File path: src/data/DataStore.ts
##########
@@ -983,6 +990,13 @@ class DataStore {
}
}
+ if (countNaN > 0 && countNaN < frameEnd - frameStart) {
+ // Append first NaN point in every bucket.
+ // It is necessary to ensure the correct order of indices.
+ newIndices[sampledIndex++] = Math.min(firstNaNIndex, nextRawIndex);
+ nextRawIndex = Math.max(firstNaNIndex, nextRawIndex);
Review comment:
This may lead to append two `NaN` values because `nextRawIndex` is same with firstNaNIndex when it's smaller.
A possibly better solution is
```ts
firstNaNIndex > nextRawIndex && (newIndices[sampledIndex++] = nextRawIndex);
if (countNaN > 0 && countNaN < frameEnd - frameStart) {
newIndices[sampledIndex++] = Math.min(firstNaNIndex, nextRawIndex);
}
// Includes the case there is no NaN value, the firstNaNIndex is -1.
firstNaNIndex <= nextRawIndex && (newIndices[sampledIndex++] = nextRawIndex);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] pissang commented on a change in pull request #16431: fix(line): using sampling with lttb, NaN point should not be filtered.
Posted by GitBox <gi...@apache.org>.
pissang commented on a change in pull request #16431:
URL: https://github.com/apache/echarts/pull/16431#discussion_r805637379
##########
File path: src/data/DataStore.ts
##########
@@ -983,6 +990,10 @@ class DataStore {
}
}
+ if (countNaN > 0 && countNaN < frameEnd - frameStart) {
Review comment:
Thanks for the explaination. Looks good
##########
File path: src/data/DataStore.ts
##########
@@ -991,7 +1002,12 @@ class DataStore {
// First frame use the last data.
newIndices[sampledIndex++] = this.getRawIndex(len - 1);
target._count = sampledIndex;
- target._indices = newIndices;
+
+ const _newIndices = new (getIndicesCtor(this._rawCount))(sampledIndex);
+ for (let idx = 0; idx < sampledIndex; idx++) {
+ _newIndices[idx] = newIndices[idx];
+ }
+ target._indices = _newIndices;
Review comment:
It's not required to create a new array which match lenght precisely.
Instead of `_indices.length`, we use `_count` to indicate the length of indices. So we can avoid creating dynamic arrays that may has worse performance than a static array.
In the code, we don't need to create a `_newIndices`. Just use the `newIndices`, it's fine even if it has empty items.
##########
File path: src/data/DataStore.ts
##########
@@ -935,7 +935,7 @@ class DataStore {
let area;
let nextRawIndex;
- const newIndices = new (getIndicesCtor(this._rawCount))(Math.ceil(len / frameSize) + 2);
+ const newIndices = new (getIndicesCtor(this._rawCount))(len);
Review comment:
Using `len` as estimated size is simple and safe, but not with best performance. From current strategy, we can know we may have at most two values in each frame(one for picked value and one for NaN value). And we use `Math.ceil(len / frameSize) + 2` to calculate the possible frames.
So following code will be more performant and also robust enough to calculate possible size when we having millions of data:
```ts
Math.min((Math.ceil(len / frameSize) + 2) * 2, len)
```
##########
File path: src/data/DataStore.ts
##########
@@ -991,7 +1002,12 @@ class DataStore {
// First frame use the last data.
newIndices[sampledIndex++] = this.getRawIndex(len - 1);
target._count = sampledIndex;
- target._indices = newIndices;
+
+ const _newIndices = new (getIndicesCtor(this._rawCount))(sampledIndex);
+ for (let idx = 0; idx < sampledIndex; idx++) {
+ _newIndices[idx] = newIndices[idx];
+ }
+ target._indices = _newIndices;
Review comment:
It's not required to create a new array which match the length precisely.
Instead of `_indices.length`, we use `_count` to indicate the length of indices. So we can avoid creating dynamic arrays that may has worse performance than a static array.
In the code, we don't need to create a `_newIndices`. Just use the `newIndices`, it's fine even if it has empty items.
##########
File path: src/data/DataStore.ts
##########
@@ -935,7 +935,7 @@ class DataStore {
let area;
let nextRawIndex;
- const newIndices = new (getIndicesCtor(this._rawCount))(Math.ceil(len / frameSize) + 2);
+ const newIndices = new (getIndicesCtor(this._rawCount))(len);
Review comment:
Using `len` as estimated size is simple and safe, but not with best performance. From current strategy, we can know we may have at most two values in each frame(one for picked value and one for NaN value). And we use `Math.ceil(len / frameSize) + 2` to calculate the possible frames.
So following code will be more performant and also robust enough to calculate the possible size when we having millions of data:
```ts
Math.min((Math.ceil(len / frameSize) + 2) * 2, len)
```
##########
File path: src/data/DataStore.ts
##########
@@ -935,7 +935,7 @@ class DataStore {
let area;
let nextRawIndex;
- const newIndices = new (getIndicesCtor(this._rawCount))(Math.ceil(len / frameSize) + 2);
+ const newIndices = new (getIndicesCtor(this._rawCount))(len);
Review comment:
Using `len` as estimated size is simple and safe, but not with best performance. From current strategy, we can know we may have at most two values in each frame(one for picked value and one for NaN value). And we use `Math.ceil(len / frameSize) + 2` to calculate the possible frames.
So when we having millions of data, following code will be more performant and also robust enough to calculate the possible size ```ts
Math.min((Math.ceil(len / frameSize) + 2) * 2, len)
```
##########
File path: src/data/DataStore.ts
##########
@@ -935,7 +935,7 @@ class DataStore {
let area;
let nextRawIndex;
- const newIndices = new (getIndicesCtor(this._rawCount))(Math.ceil(len / frameSize) + 2);
+ const newIndices = new (getIndicesCtor(this._rawCount))(len);
Review comment:
Using `len` as estimated size is simple and safe, but not with best performance. From current strategy, we can know we may have at most two values in each frame(one for picked value and one for NaN value). And we use `Math.ceil(len / frameSize) + 2` to calculate the possible frames.
So when we having millions of data, following code will be more performant and also robust enough to calculate the possible size
```ts
Math.min((Math.ceil(len / frameSize) + 2) * 2, len)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] pissang commented on pull request #16431: fix(line): using sampling with lttb, NaN point should not be filtered.
Posted by GitBox <gi...@apache.org>.
pissang commented on pull request #16431:
URL: https://github.com/apache/echarts/pull/16431#issuecomment-1041028558
I'm not sure if the behavior I described above is because we append both the NaN value and normal value. What if we only append the NaN value if there is any NaN value exists in one frame.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] pissang commented on a change in pull request #16431: fix(line): using sampling with lttb, NaN point should not be filtered.
Posted by GitBox <gi...@apache.org>.
pissang commented on a change in pull request #16431:
URL: https://github.com/apache/echarts/pull/16431#discussion_r806412003
##########
File path: src/data/DataStore.ts
##########
@@ -935,7 +935,7 @@ class DataStore {
let area;
let nextRawIndex;
- const newIndices = new (getIndicesCtor(this._rawCount))(Math.ceil(len / frameSize) + 2);
+ const newIndices = new (getIndicesCtor(this._rawCount))(len);
Review comment:
Using `len` as estimated size is simple and safe, but not with best performance. From current strategy, we can know we may have at most two values in each frame(one for picked value and one for NaN value). And we use `Math.ceil(len / frameSize) + 2` to calculate the possible frames.
So following code will be more performant and also robust enough to calculate the possible size when we having millions of data:
```ts
Math.min((Math.ceil(len / frameSize) + 2) * 2, len)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] pissang edited a comment on pull request #16431: fix(line): using sampling with lttb, NaN point should not be filtered.
Posted by GitBox <gi...@apache.org>.
pissang edited a comment on pull request #16431:
URL: https://github.com/apache/echarts/pull/16431#issuecomment-1041016471
Seems when I'm running visual regression test(which has roslution 830x750), the current behavior seems to having unexpected points in the null values.
| Before | After |
| :------------------------------------: | :----------------------------------: |
|  |  |
<img width="107" alt="image" src="https://user-images.githubusercontent.com/841551/154188257-244611ac-8399-4152-8054-e8a0e70305a5.png">
Also I think we can add the case from https://codepen.io/tmscer/pen/yLgGXbo directly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] pissang commented on a change in pull request #16431: fix(line): using sampling with lttb, NaN point should not be filtered.
Posted by GitBox <gi...@apache.org>.
pissang commented on a change in pull request #16431:
URL: https://github.com/apache/echarts/pull/16431#discussion_r803258012
##########
File path: src/data/DataStore.ts
##########
@@ -983,6 +985,10 @@ class DataStore {
}
}
+ if (appendNaNList.length && appendNaNList.length !== frameEnd - frameStart) {
+ newIndices[sampledIndex++] = appendNaNList[0]; // append first NaN point in every bucket
Review comment:
If we only append the first NaN point. We should use a variable instead of an array because array will cost more. For example:
```ts
let firstNaNIndex = -1;
if (isNaN(y) && firstNaNIndex < 0) {
firstNaNIndex = rawIndex.
)
...
if (firstNaNIndex >= 0) {
newIndices[sampledIndex++] = firstNaNIndex;
}
```
The performance is one of the most important indicator when implementing the sampling algorithm. We can check if it's degenerated in the `test/sample-compare.html` case
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] fuchunhui commented on a change in pull request #16431: fix(line): using sampling with lttb, NaN point should not be filtered.
Posted by GitBox <gi...@apache.org>.
fuchunhui commented on a change in pull request #16431:
URL: https://github.com/apache/echarts/pull/16431#discussion_r806489135
##########
File path: src/data/DataStore.ts
##########
@@ -935,7 +935,7 @@ class DataStore {
let area;
let nextRawIndex;
- const newIndices = new (getIndicesCtor(this._rawCount))(Math.ceil(len / frameSize) + 2);
+ const newIndices = new (getIndicesCtor(this._rawCount))(len);
Review comment:
good idea
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] fuchunhui commented on a change in pull request #16431: fix(line): using sampling with lttb, NaN point should not be filtered.
Posted by GitBox <gi...@apache.org>.
fuchunhui commented on a change in pull request #16431:
URL: https://github.com/apache/echarts/pull/16431#discussion_r806489135
##########
File path: src/data/DataStore.ts
##########
@@ -935,7 +935,7 @@ class DataStore {
let area;
let nextRawIndex;
- const newIndices = new (getIndicesCtor(this._rawCount))(Math.ceil(len / frameSize) + 2);
+ const newIndices = new (getIndicesCtor(this._rawCount))(len);
Review comment:
good idea
##########
File path: src/data/DataStore.ts
##########
@@ -991,7 +1002,12 @@ class DataStore {
// First frame use the last data.
newIndices[sampledIndex++] = this.getRawIndex(len - 1);
target._count = sampledIndex;
- target._indices = newIndices;
+
+ const _newIndices = new (getIndicesCtor(this._rawCount))(sampledIndex);
+ for (let idx = 0; idx < sampledIndex; idx++) {
+ _newIndices[idx] = newIndices[idx];
+ }
+ target._indices = _newIndices;
Review comment:
got it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] fuchunhui commented on pull request #16431: fix(line): using sampling with lttb, NaN point should not be filtered.
Posted by GitBox <gi...@apache.org>.
fuchunhui commented on pull request #16431:
URL: https://github.com/apache/echarts/pull/16431#issuecomment-1042858237
> Seems when I'm running visual regression test(which has roslution 830x750), the current behavior seems to having unexpected points in the null values.
>
> Before After
>  
> <img alt="image" width="107" src="https://user-images.githubusercontent.com/841551/154188257-244611ac-8399-4152-8054-e8a0e70305a5.png">
>
> Also I think we can add the case from https://codepen.io/tmscer/pen/yLgGXbo directly.
fixed. Due to incorrect index order.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] fuchunhui commented on pull request #16431: fix(line): using sampling with lttb, NaN point should not be filtered.
Posted by GitBox <gi...@apache.org>.
fuchunhui commented on pull request #16431:
URL: https://github.com/apache/echarts/pull/16431#issuecomment-1042867558
```
// set frameSize = 3；
const dimStore = [21, 22, 20, 33, 44, 55];
// IF result index is [1, 5]
```
Demo 1：
```
const dimStore = [21, null, 20, 33, 44, 55];
// Right result is [0, 1, 5]
// Before, it would be [1, 0, 5], not right, this made the wrong case as you mentioned
```
@pissang
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] pissang edited a comment on pull request #16431: fix(line): using sampling with lttb, NaN point should not be filtered.
Posted by GitBox <gi...@apache.org>.
pissang edited a comment on pull request #16431:
URL: https://github.com/apache/echarts/pull/16431#issuecomment-1041016471
Seems when I'm running visual regression test(which has roslution 830x750), the current behavior seems to having unexpected points in the null values.
Also I think we can add the case from https://codepen.io/tmscer/pen/yLgGXbo directly.
Before:
After:
<img width="107" alt="image" src="https://user-images.githubusercontent.com/841551/154188257-244611ac-8399-4152-8054-e8a0e70305a5.png">
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] fuchunhui commented on a change in pull request #16431: fix(line): using sampling with lttb, NaN point should not be filtered.
Posted by GitBox <gi...@apache.org>.
fuchunhui commented on a change in pull request #16431:
URL: https://github.com/apache/echarts/pull/16431#discussion_r804406745
##########
File path: src/data/DataStore.ts
##########
@@ -983,6 +985,10 @@ class DataStore {
}
}
+ if (appendNaNList.length && appendNaNList.length !== frameEnd - frameStart) {
+ newIndices[sampledIndex++] = appendNaNList[0]; // append first NaN point in every bucket
Review comment:
yes, got it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] pissang commented on a change in pull request #16431: fix(line): using sampling with lttb, NaN point should not be filtered.
Posted by GitBox <gi...@apache.org>.
pissang commented on a change in pull request #16431:
URL: https://github.com/apache/echarts/pull/16431#discussion_r805536323
##########
File path: src/data/DataStore.ts
##########
@@ -983,6 +990,10 @@ class DataStore {
}
}
+ if (countNaN > 0 && countNaN < frameEnd - frameStart) {
Review comment:
I'm not sure what's this `countNaN < frameEnd - frameStart` check for?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] fuchunhui commented on a change in pull request #16431: fix(line): using sampling with lttb, NaN point should not be filtered.
Posted by GitBox <gi...@apache.org>.
fuchunhui commented on a change in pull request #16431:
URL: https://github.com/apache/echarts/pull/16431#discussion_r806489999
##########
File path: src/data/DataStore.ts
##########
@@ -991,7 +1002,12 @@ class DataStore {
// First frame use the last data.
newIndices[sampledIndex++] = this.getRawIndex(len - 1);
target._count = sampledIndex;
- target._indices = newIndices;
+
+ const _newIndices = new (getIndicesCtor(this._rawCount))(sampledIndex);
+ for (let idx = 0; idx < sampledIndex; idx++) {
+ _newIndices[idx] = newIndices[idx];
+ }
+ target._indices = _newIndices;
Review comment:
got it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] pissang commented on a change in pull request #16431: fix(line): using sampling with lttb, NaN point should not be filtered.
Posted by GitBox <gi...@apache.org>.
pissang commented on a change in pull request #16431:
URL: https://github.com/apache/echarts/pull/16431#discussion_r806410399
##########
File path: src/data/DataStore.ts
##########
@@ -991,7 +1002,12 @@ class DataStore {
// First frame use the last data.
newIndices[sampledIndex++] = this.getRawIndex(len - 1);
target._count = sampledIndex;
- target._indices = newIndices;
+
+ const _newIndices = new (getIndicesCtor(this._rawCount))(sampledIndex);
+ for (let idx = 0; idx < sampledIndex; idx++) {
+ _newIndices[idx] = newIndices[idx];
+ }
+ target._indices = _newIndices;
Review comment:
It's not required to create a new array which match the length precisely.
Instead of `_indices.length`, we use `_count` to indicate the length of indices. So we can avoid creating dynamic arrays that may has worse performance than a static array.
In the code, we don't need to create a `_newIndices`. Just use the `newIndices`, it's fine even if it has empty items.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] pissang commented on a change in pull request #16431: fix(line): using sampling with lttb, NaN point should not be filtered.
Posted by GitBox <gi...@apache.org>.
pissang commented on a change in pull request #16431:
URL: https://github.com/apache/echarts/pull/16431#discussion_r805637379
##########
File path: src/data/DataStore.ts
##########
@@ -983,6 +990,10 @@ class DataStore {
}
}
+ if (countNaN > 0 && countNaN < frameEnd - frameStart) {
Review comment:
Thanks for the explaination. Looks good
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] pissang merged pull request #16431: fix(line): using sampling with lttb, NaN point should not be filtered.
Posted by GitBox <gi...@apache.org>.
pissang merged pull request #16431:
URL: https://github.com/apache/echarts/pull/16431
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] echarts-bot[bot] commented on pull request #16431: fix(line): using sampling with lttb, NaN point should not be filtered.
Posted by GitBox <gi...@apache.org>.
echarts-bot[bot] commented on pull request #16431:
URL: https://github.com/apache/echarts/pull/16431#issuecomment-1021878510
Thanks for your contribution!
The community will review it ASAP. In the meanwhile, please checkout [the coding standard](https://echarts.apache.org/en/coding-standard.html) and Wiki about [How to make a pull request](https://github.com/apache/echarts/wiki/How-to-make-a-pull-request).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] pissang commented on a change in pull request #16431: fix(line): using sampling with lttb, NaN point should not be filtered.
Posted by GitBox <gi...@apache.org>.
pissang commented on a change in pull request #16431:
URL: https://github.com/apache/echarts/pull/16431#discussion_r805536323
##########
File path: src/data/DataStore.ts
##########
@@ -983,6 +990,10 @@ class DataStore {
}
}
+ if (countNaN > 0 && countNaN < frameEnd - frameStart) {
Review comment:
I'm not sure what's this `countNaN < frameEnd - frameStart` check for? Do you mind explaining the strategy used here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] fuchunhui commented on a change in pull request #16431: fix(line): using sampling with lttb, NaN point should not be filtered.
Posted by GitBox <gi...@apache.org>.
fuchunhui commented on a change in pull request #16431:
URL: https://github.com/apache/echarts/pull/16431#discussion_r805632787
##########
File path: src/data/DataStore.ts
##########
@@ -983,6 +990,10 @@ class DataStore {
}
}
+ if (countNaN > 0 && countNaN < frameEnd - frameStart) {
Review comment:
In a bucket, if all data is NaN, execute the original logic without separate processing. `countNaN < frameEnd - frameStart` can avoid adding multiple times.
```
// set frameSize = 3；
const dimStore = [11, 22, 20, 33, 44, 55];
// IF result index is [1, 5]
```
Demo 1:
```
const dimStore = [null, 22, 20, 33, 44, 55];
// result index is [0, 1, 5]
```
Demo 2:
```
const dimStore = [null, null, null, 33, 44, 55];
// result index is [0, 5]
// if not check, result will become [0, 0, 5].
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] pissang commented on a change in pull request #16431: fix(line): using sampling with lttb, NaN point should not be filtered.
Posted by GitBox <gi...@apache.org>.
pissang commented on a change in pull request #16431:
URL: https://github.com/apache/echarts/pull/16431#discussion_r806410399
##########
File path: src/data/DataStore.ts
##########
@@ -991,7 +1002,12 @@ class DataStore {
// First frame use the last data.
newIndices[sampledIndex++] = this.getRawIndex(len - 1);
target._count = sampledIndex;
- target._indices = newIndices;
+
+ const _newIndices = new (getIndicesCtor(this._rawCount))(sampledIndex);
+ for (let idx = 0; idx < sampledIndex; idx++) {
+ _newIndices[idx] = newIndices[idx];
+ }
+ target._indices = _newIndices;
Review comment:
It's not required to create a new array which match lenght precisely.
Instead of `_indices.length`, we use `_count` to indicate the length of indices. So we can avoid creating dynamic arrays that may has worse performance than a static array.
In the code, we don't need to create a `_newIndices`. Just use the `newIndices`, it's fine even if it has empty items.
##########
File path: src/data/DataStore.ts
##########
@@ -935,7 +935,7 @@ class DataStore {
let area;
let nextRawIndex;
- const newIndices = new (getIndicesCtor(this._rawCount))(Math.ceil(len / frameSize) + 2);
+ const newIndices = new (getIndicesCtor(this._rawCount))(len);
Review comment:
Using `len` as estimated size is simple and safe, but not with best performance. From current strategy, we can know we may have at most two values in each frame(one for picked value and one for NaN value). And we use `Math.ceil(len / frameSize) + 2` to calculate the possible frames.
So following code will be more performant and also robust enough to calculate possible size when we having millions of data:
```ts
Math.min((Math.ceil(len / frameSize) + 2) * 2, len)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org
[GitHub] [echarts] pissang commented on pull request #16431: fix(line): using sampling with lttb, NaN point should not be filtered.
Posted by GitBox <gi...@apache.org>.
pissang commented on pull request #16431:
URL: https://github.com/apache/echarts/pull/16431#issuecomment-1041016471
I'm not sure but when I'm running visual regression test(which has roslution 830x750), the current behavior seems to having unexpected points in the null values.
Before:
After:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org