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 2020/10/19 14:21:04 UTC
[GitHub] [incubator-echarts] susiwen8 opened a new pull request #13210: Feat: multiple value axis alignment
susiwen8 opened a new pull request #13210:
URL: https://github.com/apache/incubator-echarts/pull/13210
<!-- Please fill in the following information to help us review your PR more efficiently. -->
## Brief Information
This pull request is in the type of:
- [ ] bug fixing
- [x] new feature
- [ ] others
### What does this PR do?
Multiple value axis alignment
### Fixed issues
Close #11390
Close #10928
## Details
`calAxisNiceSplitNumber`: this function calculates `nice split` of each `value` axis. Because user can specify `min` or `max` to `axis`, `axis` might not be cut evenly. It returns an array of `nice split`, find out the biggest one, apply to all `axis`, ideally, each `axis` has same `nice split` number (in most case which some option haven't been specified like `min`, `max`, `minInterval` etc ).
`resetAxisExtent`: this function use `calAxisNiceSplitNumber` result to set `scale` again, then get `axis` final split number(`finalSplitNumber`)
`adjustValueAxes`: this function use biggest `finalSplitNumber` to calculate `unionExtent` which represent one unit split's length, then base on `axis scale extent` and `axis scale interval`, we can get that in this grid each `axis` proportion. multiply by `unionExtent`, it's final `axis extent`
<img width="661" alt="Screen Shot 2020-09-15 at 23 43 28" src="https://user-images.githubusercontent.com/20318608/93233195-62f75280-f7ad-11ea-8fa2-182840b83abb.png">
<img width="656" alt="Screen Shot 2020-09-15 at 23 43 36" src="https://user-images.githubusercontent.com/20318608/93233218-67bc0680-f7ad-11ea-9fb9-d80121d4d28c.png">
<img width="632" alt="Screen Shot 2020-09-15 at 23 43 49" src="https://user-images.githubusercontent.com/20318608/93233226-6ab6f700-f7ad-11ea-867a-a707fded9b9a.png">
## Usage
### Are there any API changes?
- [ ] The API has been changed.
<!-- LIST THE API CHANGES HERE -->
### Related test cases or examples to use the new APIs
NA.
## Others
### Merging options
- [ ] Please squash the commits into a single one when merge.
### TODO
- [ ] work with `min`
----------------------------------------------------------------
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.
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] [incubator-echarts] susiwen8 commented on a change in pull request #13210: Feat: multiple value axis alignment
Posted by GitBox <gi...@apache.org>.
susiwen8 commented on a change in pull request #13210:
URL: https://github.com/apache/incubator-echarts/pull/13210#discussion_r492436816
##########
File path: src/component/axis/AxisBuilder.ts
##########
@@ -372,7 +372,7 @@ const builders: Record<'axisLine' | 'axisTickLabel' | 'axisName', AxisElementsBu
const textStyleModel = axisModel.getModel('nameTextStyle');
const gap = axisModel.get('nameGap') || 0;
- const extent = axisModel.axis.getExtent();
+ const extent = axisModel.axis.getGridExtent();
const gapSignal = extent[0] > extent[1] ? -1 : 1;
Review comment:
Sorry for this late reply
The reason why `splitline` of two axis cannot align in most case, it's because axis will fill the `grid`[code](https://github.com/apache/incubator-echarts/blob/next/src/coord/cartesian/Grid.ts#L166). `axis` will be split evenly by using `grid extent`. What I trying to do is calculate the `proportion` of axis in this grid, each axis has same standard which is `unionExtent`.
##########
File path: src/component/axis/AxisBuilder.ts
##########
@@ -372,7 +372,7 @@ const builders: Record<'axisLine' | 'axisTickLabel' | 'axisName', AxisElementsBu
const textStyleModel = axisModel.getModel('nameTextStyle');
const gap = axisModel.get('nameGap') || 0;
- const extent = axisModel.axis.getExtent();
+ const extent = axisModel.axis.getGridExtent();
const gapSignal = extent[0] > extent[1] ? -1 : 1;
Review comment:
@pissang So, if user specified `min` or `max`, `axis extent` will be different from `grid extent`.
##########
File path: src/component/axis/AxisBuilder.ts
##########
@@ -372,7 +372,7 @@ const builders: Record<'axisLine' | 'axisTickLabel' | 'axisName', AxisElementsBu
const textStyleModel = axisModel.getModel('nameTextStyle');
const gap = axisModel.get('nameGap') || 0;
- const extent = axisModel.axis.getExtent();
+ const extent = axisModel.axis.getGridExtent();
const gapSignal = extent[0] > extent[1] ? -1 : 1;
Review comment:
Sorry for this late reply
The reason why `splitline` of two axis cannot align in most case, it's because axis will fill the `grid`[see here](https://github.com/apache/incubator-echarts/blob/next/src/coord/cartesian/Grid.ts#L166). `axis` will be split evenly by using `grid extent`. What I trying to do is calculate the `proportion` of axis in this grid, each axis has same standard which is `unionExtent`.
##########
File path: src/component/axis/AxisBuilder.ts
##########
@@ -372,7 +372,7 @@ const builders: Record<'axisLine' | 'axisTickLabel' | 'axisName', AxisElementsBu
const textStyleModel = axisModel.getModel('nameTextStyle');
const gap = axisModel.get('nameGap') || 0;
- const extent = axisModel.axis.getExtent();
+ const extent = axisModel.axis.getGridExtent();
const gapSignal = extent[0] > extent[1] ? -1 : 1;
Review comment:
@pissang That's correct, because without `min` `max`, axis will fill the `grid`, the proportion will be `100%`, with `min` or `max`, it will be less than `100%`(in most case, some case can still have `100%`, `max % interval === 0` ).
----------------------------------------------------------------
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.
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] [incubator-echarts] pissang edited a comment on pull request #13210: Feat: multiple value axis alignment
Posted by GitBox <gi...@apache.org>.
pissang edited a comment on pull request #13210:
URL: https://github.com/apache/incubator-echarts/pull/13210#issuecomment-712553970
It's strange that the top and bottom ticks of two axes don't align.
----------------------------------------------------------------
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.
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] [incubator-echarts] pissang commented on pull request #13210: Feat: multiple value axis alignment
Posted by GitBox <gi...@apache.org>.
pissang commented on pull request #13210:
URL: https://github.com/apache/incubator-echarts/pull/13210#issuecomment-712195527
Seems the last two cases are broken.
----------------------------------------------------------------
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.
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] [incubator-echarts] pissang commented on pull request #13210: Feat: multiple value axis alignment
Posted by GitBox <gi...@apache.org>.
pissang commented on pull request #13210:
URL: https://github.com/apache/incubator-echarts/pull/13210#issuecomment-712553970
It's strange that the top and bottom ticks didn't align.
----------------------------------------------------------------
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.
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] [incubator-echarts] susiwen8 edited a comment on pull request #13210: Feat: multiple value axis alignment
Posted by GitBox <gi...@apache.org>.
susiwen8 edited a comment on pull request #13210:
URL: https://github.com/apache/incubator-echarts/pull/13210#issuecomment-712229341
Right now, this doesn't work with `min`. Strange result from `dataToCoord`
----------------------------------------------------------------
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.
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] [incubator-echarts] pissang commented on a change in pull request #13210: Feat: multiple value axis alignment
Posted by GitBox <gi...@apache.org>.
pissang commented on a change in pull request #13210:
URL: https://github.com/apache/incubator-echarts/pull/13210#discussion_r492453250
##########
File path: src/component/axis/AxisBuilder.ts
##########
@@ -372,7 +372,7 @@ const builders: Record<'axisLine' | 'axisTickLabel' | 'axisName', AxisElementsBu
const textStyleModel = axisModel.getModel('nameTextStyle');
const gap = axisModel.get('nameGap') || 0;
- const extent = axisModel.axis.getExtent();
+ const extent = axisModel.axis.getGridExtent();
const gapSignal = extent[0] > extent[1] ? -1 : 1;
Review comment:
I thought the main reason two axes don't align is that they don't have the same split numbers. They should be aligned if the split numbers are same.
##########
File path: src/component/axis/AxisBuilder.ts
##########
@@ -372,7 +372,7 @@ const builders: Record<'axisLine' | 'axisTickLabel' | 'axisName', AxisElementsBu
const textStyleModel = axisModel.getModel('nameTextStyle');
const gap = axisModel.get('nameGap') || 0;
- const extent = axisModel.axis.getExtent();
+ const extent = axisModel.axis.getGridExtent();
const gapSignal = extent[0] > extent[1] ? -1 : 1;
Review comment:
<del>I thought the main reason two axes don't align is that they don't have the same split numbers. They should be aligned if the split numbers are same.</del>
I see, axis extent is different from grid extent only when `min`, `max` is fixed?
##########
File path: src/component/axis/AxisBuilder.ts
##########
@@ -372,7 +372,7 @@ const builders: Record<'axisLine' | 'axisTickLabel' | 'axisName', AxisElementsBu
const textStyleModel = axisModel.getModel('nameTextStyle');
const gap = axisModel.get('nameGap') || 0;
- const extent = axisModel.axis.getExtent();
+ const extent = axisModel.axis.getGridExtent();
const gapSignal = extent[0] > extent[1] ? -1 : 1;
Review comment:
<del>I thought the main reason two axes don't align is that they don't have the same split numbers. They should be aligned if the split numbers are same.</del>
I see, so axis extent is different from grid extent only when `min`, `max` is fixed?
----------------------------------------------------------------
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.
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] [incubator-echarts] pissang edited a comment on pull request #13210: Feat: multiple value axis alignment
Posted by GitBox <gi...@apache.org>.
pissang edited a comment on pull request #13210:
URL: https://github.com/apache/incubator-echarts/pull/13210#issuecomment-712195527
Seems the last two cases are broken.
----------------------------------------------------------------
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.
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] [incubator-echarts] susiwen8 commented on pull request #13210: Feat: multiple value axis alignment
Posted by GitBox <gi...@apache.org>.
susiwen8 commented on pull request #13210:
URL: https://github.com/apache/incubator-echarts/pull/13210#issuecomment-712221103
> Seems the last two cases are broken.
>
> 
@pissang No, those were the line for `max`
<img width="278" alt="Screen Shot 2020-10-19 at 22 52 27" src="https://user-images.githubusercontent.com/20318608/96467717-cb7a9900-125d-11eb-8d6b-3937bc6a1951.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.
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] [incubator-echarts] susiwen8 commented on a change in pull request #13210: Feat: multiple value axis alignment
Posted by GitBox <gi...@apache.org>.
susiwen8 commented on a change in pull request #13210:
URL: https://github.com/apache/incubator-echarts/pull/13210#discussion_r492436816
##########
File path: src/component/axis/AxisBuilder.ts
##########
@@ -372,7 +372,7 @@ const builders: Record<'axisLine' | 'axisTickLabel' | 'axisName', AxisElementsBu
const textStyleModel = axisModel.getModel('nameTextStyle');
const gap = axisModel.get('nameGap') || 0;
- const extent = axisModel.axis.getExtent();
+ const extent = axisModel.axis.getGridExtent();
const gapSignal = extent[0] > extent[1] ? -1 : 1;
Review comment:
Sorry for this late reply
The reason why `splitline` of two axis cannot align in most case, it's because axis will fill the `grid`[see here](https://github.com/apache/incubator-echarts/blob/next/src/coord/cartesian/Grid.ts#L166). `axis` will be split evenly by using `grid extent`. What I trying to do is calculate the `proportion` of axis in this grid, each axis has same standard which is `unionExtent`.
----------------------------------------------------------------
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.
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] [incubator-echarts] pissang commented on a change in pull request #13210: Feat: multiple value axis alignment
Posted by GitBox <gi...@apache.org>.
pissang commented on a change in pull request #13210:
URL: https://github.com/apache/incubator-echarts/pull/13210#discussion_r489138320
##########
File path: src/coord/Axis.ts
##########
@@ -106,6 +107,14 @@ class Axis {
);
}
+ setGridExtent(start: number, end: number) {
+ this._gridExtent = [start, end];
+ }
+
+ getGridExtent(): [number, number] {
+ return this._gridExtent.slice() as [number, number];
+ }
+
Review comment:
As described above. The concept `grid` should not be leaked to the very general Axis module.
##########
File path: src/component/axis/AxisBuilder.ts
##########
@@ -372,7 +372,7 @@ const builders: Record<'axisLine' | 'axisTickLabel' | 'axisName', AxisElementsBu
const textStyleModel = axisModel.getModel('nameTextStyle');
const gap = axisModel.get('nameGap') || 0;
- const extent = axisModel.axis.getExtent();
+ const extent = axisModel.axis.getGridExtent();
const gapSignal = extent[0] > extent[1] ? -1 : 1;
Review comment:
AxisBuilder will also be used on polar, parallel. Not only on grid.
Also, I'm still not sure about the difference between the grid extent and axis extent. When will these two extents has different values?
##########
File path: src/coord/cartesian/Grid.ts
##########
@@ -85,18 +86,68 @@ class Grid implements CoordinateSystemMaster {
return this._rect;
}
+ calAxisNiceSplitNumber(axes: Axis2D[]) {
+ const splitNumber: number[] = [];
+ const niceAxisExtents: number[] = [];
+ each(axes, axis => {
+ niceScaleExtent(axis.scale, axis.model);
+ if (axis.type !== 'value') {
+ return;
+ }
+ const extent = axis.scale.getExtent();
+ const interval = (axis.scale as IntervalScale).getInterval();
+ splitNumber.push(
+ Math.floor((extent[1] - extent[0]) / interval)
+ );
+ const axisExtent = axis.getExtent();
+ niceAxisExtents.push((axisExtent[1] - axisExtent[0]) * interval / extent[1]);
+ });
+
+ return splitNumber;
+ }
+
+ resetAxisExtent(axes: Axis2D[], splitNumber: number[], maxSplitNumber: number): number[] {
+ const finalSplitNumber: number[] = [];
+ each(axes, function (axis, index) {
+ if (axis.type !== 'value') {
+ return;
+ }
+
+ if (!(maxSplitNumber - splitNumber[index])) {
+ finalSplitNumber.push(maxSplitNumber);
+ return;
+ };
+
+ const extent = axis.scale.getExtent();
+ const interval = (axis.scale as IntervalScale).getInterval();
+ if (((extent[1] - extent[0]) % interval) === 0) {
+ extent[1] = extent[1] + (maxSplitNumber - splitNumber[index]) * interval;
+ niceScaleExtent(axis.scale, axis.model, extent, maxSplitNumber);
+ }
+ else {
+ (axis.scale as IntervalScale).niceTicks(
+ maxSplitNumber + 1, null, null,
+ ((extent[1] - extent[0]) - ((extent[1] - extent[0]) % maxSplitNumber)) / maxSplitNumber
+ );
Review comment:
`extent[1] - extent[0]` should be rounded here to avoid rounding error.
```js
numberUtil.round(extent[1] - extent[0]);
```
The result interval should also be rounded.
##########
File path: src/coord/cartesian/Grid.ts
##########
@@ -85,18 +86,68 @@ class Grid implements CoordinateSystemMaster {
return this._rect;
}
+ calAxisNiceSplitNumber(axes: Axis2D[]) {
+ const splitNumber: number[] = [];
+ const niceAxisExtents: number[] = [];
+ each(axes, axis => {
+ niceScaleExtent(axis.scale, axis.model);
+ if (axis.type !== 'value') {
+ return;
+ }
+ const extent = axis.scale.getExtent();
+ const interval = (axis.scale as IntervalScale).getInterval();
+ splitNumber.push(
+ Math.floor((extent[1] - extent[0]) / interval)
+ );
+ const axisExtent = axis.getExtent();
+ niceAxisExtents.push((axisExtent[1] - axisExtent[0]) * interval / extent[1]);
+ });
+
+ return splitNumber;
+ }
+
+ resetAxisExtent(axes: Axis2D[], splitNumber: number[], maxSplitNumber: number): number[] {
+ const finalSplitNumber: number[] = [];
+ each(axes, function (axis, index) {
+ if (axis.type !== 'value') {
+ return;
+ }
+
+ if (!(maxSplitNumber - splitNumber[index])) {
+ finalSplitNumber.push(maxSplitNumber);
+ return;
+ };
+
+ const extent = axis.scale.getExtent();
+ const interval = (axis.scale as IntervalScale).getInterval();
+ if (((extent[1] - extent[0]) % interval) === 0) {
Review comment:
`(extent[1] - extent[0]) % interval` is not very robust because of precision issue. For example:
`(0.3 - 0.1) % 0.1` will get `0.09999999999999998`.
Should use a more numeric robust way:
```js
const splitNumber = (extent[1] - extent[0]) / interval;
// Is integer
if (Math.abs(Math.round(splitNumber) - splitNumber)) < 1e-8) {
....
}
```
##########
File path: src/coord/cartesian/Grid.ts
##########
@@ -85,18 +86,68 @@ class Grid implements CoordinateSystemMaster {
return this._rect;
}
+ calAxisNiceSplitNumber(axes: Axis2D[]) {
+ const splitNumber: number[] = [];
+ const niceAxisExtents: number[] = [];
+ each(axes, axis => {
+ niceScaleExtent(axis.scale, axis.model);
+ if (axis.type !== 'value') {
+ return;
+ }
+ const extent = axis.scale.getExtent();
+ const interval = (axis.scale as IntervalScale).getInterval();
+ splitNumber.push(
+ Math.floor((extent[1] - extent[0]) / interval)
+ );
+ const axisExtent = axis.getExtent();
+ niceAxisExtents.push((axisExtent[1] - axisExtent[0]) * interval / extent[1]);
+ });
+
+ return splitNumber;
+ }
+
+ resetAxisExtent(axes: Axis2D[], splitNumber: number[], maxSplitNumber: number): number[] {
+ const finalSplitNumber: number[] = [];
+ each(axes, function (axis, index) {
+ if (axis.type !== 'value') {
+ return;
+ }
+
+ if (!(maxSplitNumber - splitNumber[index])) {
+ finalSplitNumber.push(maxSplitNumber);
+ return;
+ };
+
+ const extent = axis.scale.getExtent();
+ const interval = (axis.scale as IntervalScale).getInterval();
+ if (((extent[1] - extent[0]) % interval) === 0) {
+ extent[1] = extent[1] + (maxSplitNumber - splitNumber[index]) * interval;
Review comment:
Should be `extent[1] += (maxSplitNumber - splitNumber[index]) * interval` here?
Also, I'm wondering if there is a case that all the values are negative. And the extent is [-10, 0]. In this case, developers may prefer `extent[0] -= (maxSplitNumber - splitNumber[index]) * interval`
##########
File path: src/scale/helper.ts
##########
@@ -35,13 +35,14 @@ export function intervalScaleNiceTicks(
extent: [number, number],
splitNumber: number,
minInterval?: number,
- maxInterval?: number
+ maxInterval?: number,
+ interval?: number
): intervalScaleNiceTicksResult {
Review comment:
Perhaps we can change the parameter name to `preferredInterval`. It's more descriptive and we don't need to modify the argument.
##########
File path: src/coord/cartesian/Grid.ts
##########
@@ -85,18 +86,68 @@ class Grid implements CoordinateSystemMaster {
return this._rect;
}
+ calAxisNiceSplitNumber(axes: Axis2D[]) {
+ const splitNumber: number[] = [];
+ const niceAxisExtents: number[] = [];
+ each(axes, axis => {
+ niceScaleExtent(axis.scale, axis.model);
+ if (axis.type !== 'value') {
+ return;
+ }
+ const extent = axis.scale.getExtent();
+ const interval = (axis.scale as IntervalScale).getInterval();
+ splitNumber.push(
+ Math.floor((extent[1] - extent[0]) / interval)
+ );
+ const axisExtent = axis.getExtent();
+ niceAxisExtents.push((axisExtent[1] - axisExtent[0]) * interval / extent[1]);
+ });
+
+ return splitNumber;
+ }
+
+ resetAxisExtent(axes: Axis2D[], splitNumber: number[], maxSplitNumber: number): number[] {
+ const finalSplitNumber: number[] = [];
+ each(axes, function (axis, index) {
+ if (axis.type !== 'value') {
+ return;
+ }
+
+ if (!(maxSplitNumber - splitNumber[index])) {
+ finalSplitNumber.push(maxSplitNumber);
+ return;
+ };
+
+ const extent = axis.scale.getExtent();
+ const interval = (axis.scale as IntervalScale).getInterval();
+ if (((extent[1] - extent[0]) % interval) === 0) {
+ extent[1] = extent[1] + (maxSplitNumber - splitNumber[index]) * interval;
+ niceScaleExtent(axis.scale, axis.model, extent, maxSplitNumber);
+ }
+ else {
+ (axis.scale as IntervalScale).niceTicks(
+ maxSplitNumber + 1, null, null,
+ ((extent[1] - extent[0]) - ((extent[1] - extent[0]) % maxSplitNumber)) / maxSplitNumber
+ );
+ }
+ const finalScaleExtent = axis.scale.getExtent();
+ const finalInterval = (axis.scale as IntervalScale).getInterval();
+ finalSplitNumber.push(Math.ceil((finalScaleExtent[1] - finalScaleExtent[0]) / finalInterval));
+ });
+
+ return finalSplitNumber;
+ }
+
update(ecModel: GlobalModel, api: ExtensionAPI): void {
const axesMap = this._axesMap;
this._updateScale(ecModel, this.model);
- each(axesMap.x, function (xAxis) {
- niceScaleExtent(xAxis.scale, xAxis.model);
- });
- each(axesMap.y, function (yAxis) {
- niceScaleExtent(yAxis.scale, yAxis.model);
- });
+ const niceSplitNumX = this.calAxisNiceSplitNumber(axesMap.x);
+ const niceSplitNumY = this.calAxisNiceSplitNumber(axesMap.y);
+ const finalSplitNumberY = this.resetAxisExtent(axesMap.y, niceSplitNumY, Math.max(...niceSplitNumY));
+ const finalSplitNumberX = this.resetAxisExtent(axesMap.x, niceSplitNumX, Math.max(...niceSplitNumX));
Review comment:
Seems the extent will be modified by force if the other axis is not aligned.
But in some cases, developers may fix the `min`, `max`, or even `interval`. Theses specified values should have higher priority and won't be modified.
##########
File path: src/scale/Interval.ts
##########
@@ -215,7 +215,7 @@ class IntervalScale extends Scale {
/**
* @param splitNumber By default `5`.
*/
- niceTicks(splitNumber?: number, minInterval?: number, maxInterval?: number): void {
+ niceTicks(splitNumber?: number, minInterval?: number, maxInterval?: number, interval?: number): void {
splitNumber = splitNumber || 5;
Review comment:
I will prefer using the name `preferredInterval` 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.
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] [incubator-echarts] pissang edited a comment on pull request #13210: Feat: multiple value axis alignment
Posted by GitBox <gi...@apache.org>.
pissang edited a comment on pull request #13210:
URL: https://github.com/apache/incubator-echarts/pull/13210#issuecomment-712553970
----------------------------------------------------------------
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.
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] [incubator-echarts] pissang closed pull request #13210: Feat: multiple value axis alignment
Posted by GitBox <gi...@apache.org>.
pissang closed pull request #13210:
URL: https://github.com/apache/incubator-echarts/pull/13210
----------------------------------------------------------------
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.
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] [incubator-echarts] susiwen8 commented on pull request #13210: Feat: multiple value axis alignment
Posted by GitBox <gi...@apache.org>.
susiwen8 commented on pull request #13210:
URL: https://github.com/apache/incubator-echarts/pull/13210#issuecomment-712229341
Right now, this doesn't work with `min`. Strange calculate result from `dataToCoord`
----------------------------------------------------------------
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.
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] [incubator-echarts] pissang edited a comment on pull request #13210: Feat: multiple value axis alignment
Posted by GitBox <gi...@apache.org>.
pissang edited a comment on pull request #13210:
URL: https://github.com/apache/incubator-echarts/pull/13210#issuecomment-712553970
It's strange that the top and bottom ticks of two axes don't align.
I'd suggest adding more cases to define the expected aligning behavior when different axis options `min` / `max` / `splitNumber` / `interval` / `scale` / `type` are set.
----------------------------------------------------------------
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.
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] [incubator-echarts] susiwen8 commented on a change in pull request #13210: Feat: multiple value axis alignment
Posted by GitBox <gi...@apache.org>.
susiwen8 commented on a change in pull request #13210:
URL: https://github.com/apache/incubator-echarts/pull/13210#discussion_r492436816
##########
File path: src/component/axis/AxisBuilder.ts
##########
@@ -372,7 +372,7 @@ const builders: Record<'axisLine' | 'axisTickLabel' | 'axisName', AxisElementsBu
const textStyleModel = axisModel.getModel('nameTextStyle');
const gap = axisModel.get('nameGap') || 0;
- const extent = axisModel.axis.getExtent();
+ const extent = axisModel.axis.getGridExtent();
const gapSignal = extent[0] > extent[1] ? -1 : 1;
Review comment:
Sorry for this late reply
The reason why `splitline` of two axis cannot align in most case, it's because axis will fill the `grid`[code](https://github.com/apache/incubator-echarts/blob/next/src/coord/cartesian/Grid.ts#L166). `axis` will be split evenly by using `grid extent`. What I trying to do is calculate the `proportion` of axis in this grid, each axis has same standard which is `unionExtent`.
----------------------------------------------------------------
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.
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] [incubator-echarts] susiwen8 commented on pull request #13210: Feat: multiple value axis alignment
Posted by GitBox <gi...@apache.org>.
susiwen8 commented on pull request #13210:
URL: https://github.com/apache/incubator-echarts/pull/13210#issuecomment-712655709
@pissang More test cases added.
----------------------------------------------------------------
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.
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] [incubator-echarts] susiwen8 commented on a change in pull request #13210: Feat: multiple value axis alignment
Posted by GitBox <gi...@apache.org>.
susiwen8 commented on a change in pull request #13210:
URL: https://github.com/apache/incubator-echarts/pull/13210#discussion_r494289507
##########
File path: src/coord/cartesian/Grid.ts
##########
@@ -85,18 +86,68 @@ class Grid implements CoordinateSystemMaster {
return this._rect;
}
+ calAxisNiceSplitNumber(axes: Axis2D[]) {
+ const splitNumber: number[] = [];
+ const niceAxisExtents: number[] = [];
+ each(axes, axis => {
+ niceScaleExtent(axis.scale, axis.model);
+ if (axis.type !== 'value') {
+ return;
+ }
+ const extent = axis.scale.getExtent();
+ const interval = (axis.scale as IntervalScale).getInterval();
+ splitNumber.push(
+ Math.floor((extent[1] - extent[0]) / interval)
+ );
+ const axisExtent = axis.getExtent();
+ niceAxisExtents.push((axisExtent[1] - axisExtent[0]) * interval / extent[1]);
+ });
+
+ return splitNumber;
+ }
+
+ resetAxisExtent(axes: Axis2D[], splitNumber: number[], maxSplitNumber: number): number[] {
+ const finalSplitNumber: number[] = [];
+ each(axes, function (axis, index) {
+ if (axis.type !== 'value') {
+ return;
+ }
+
+ if (!(maxSplitNumber - splitNumber[index])) {
+ finalSplitNumber.push(maxSplitNumber);
+ return;
+ };
+
+ const extent = axis.scale.getExtent();
+ const interval = (axis.scale as IntervalScale).getInterval();
+ if (((extent[1] - extent[0]) % interval) === 0) {
+ extent[1] = extent[1] + (maxSplitNumber - splitNumber[index]) * interval;
+ niceScaleExtent(axis.scale, axis.model, extent, maxSplitNumber);
+ }
+ else {
+ (axis.scale as IntervalScale).niceTicks(
+ maxSplitNumber + 1, null, null,
+ ((extent[1] - extent[0]) - ((extent[1] - extent[0]) % maxSplitNumber)) / maxSplitNumber
+ );
+ }
+ const finalScaleExtent = axis.scale.getExtent();
+ const finalInterval = (axis.scale as IntervalScale).getInterval();
+ finalSplitNumber.push(Math.ceil((finalScaleExtent[1] - finalScaleExtent[0]) / finalInterval));
+ });
+
+ return finalSplitNumber;
+ }
+
update(ecModel: GlobalModel, api: ExtensionAPI): void {
const axesMap = this._axesMap;
this._updateScale(ecModel, this.model);
- each(axesMap.x, function (xAxis) {
- niceScaleExtent(xAxis.scale, xAxis.model);
- });
- each(axesMap.y, function (yAxis) {
- niceScaleExtent(yAxis.scale, yAxis.model);
- });
+ const niceSplitNumX = this.calAxisNiceSplitNumber(axesMap.x);
+ const niceSplitNumY = this.calAxisNiceSplitNumber(axesMap.y);
+ const finalSplitNumberY = this.resetAxisExtent(axesMap.y, niceSplitNumY, Math.max(...niceSplitNumY));
+ const finalSplitNumberX = this.resetAxisExtent(axesMap.x, niceSplitNumX, Math.max(...niceSplitNumX));
Review comment:
Yes, it will change option
I will look into 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.
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] [incubator-echarts] susiwen8 commented on pull request #13210: Feat: multiple value axis alignment
Posted by GitBox <gi...@apache.org>.
susiwen8 commented on pull request #13210:
URL: https://github.com/apache/incubator-echarts/pull/13210#issuecomment-698966245
Hi @pissang I have solved the problem you mentioned, Could you review this please?
----------------------------------------------------------------
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.
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] [incubator-echarts] pissang commented on pull request #13210: Feat: multiple value axis alignment
Posted by GitBox <gi...@apache.org>.
pissang commented on pull request #13210:
URL: https://github.com/apache/incubator-echarts/pull/13210#issuecomment-693160312
The result looks good! I've left some of my questions and suggestions.
----------------------------------------------------------------
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.
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] [incubator-echarts] susiwen8 commented on a change in pull request #13210: Feat: multiple value axis alignment
Posted by GitBox <gi...@apache.org>.
susiwen8 commented on a change in pull request #13210:
URL: https://github.com/apache/incubator-echarts/pull/13210#discussion_r492436816
##########
File path: src/component/axis/AxisBuilder.ts
##########
@@ -372,7 +372,7 @@ const builders: Record<'axisLine' | 'axisTickLabel' | 'axisName', AxisElementsBu
const textStyleModel = axisModel.getModel('nameTextStyle');
const gap = axisModel.get('nameGap') || 0;
- const extent = axisModel.axis.getExtent();
+ const extent = axisModel.axis.getGridExtent();
const gapSignal = extent[0] > extent[1] ? -1 : 1;
Review comment:
Sorry for this late reply
The reason why `splitline` of two axis cannot align in most case, it's because axis will fill the `grid`[code](https://github.com/apache/incubator-echarts/blob/next/src/coord/cartesian/Grid.ts#L166). `axis` will be split evenly by using `grid extent`. What I trying to do is calculate the `proportion` of axis in this grid, each axis has same standard which is `unionExtent`.
##########
File path: src/component/axis/AxisBuilder.ts
##########
@@ -372,7 +372,7 @@ const builders: Record<'axisLine' | 'axisTickLabel' | 'axisName', AxisElementsBu
const textStyleModel = axisModel.getModel('nameTextStyle');
const gap = axisModel.get('nameGap') || 0;
- const extent = axisModel.axis.getExtent();
+ const extent = axisModel.axis.getGridExtent();
const gapSignal = extent[0] > extent[1] ? -1 : 1;
Review comment:
@pissang So, if user specified `min` or `max`, `axis extent` will be different from `grid extent`.
##########
File path: src/component/axis/AxisBuilder.ts
##########
@@ -372,7 +372,7 @@ const builders: Record<'axisLine' | 'axisTickLabel' | 'axisName', AxisElementsBu
const textStyleModel = axisModel.getModel('nameTextStyle');
const gap = axisModel.get('nameGap') || 0;
- const extent = axisModel.axis.getExtent();
+ const extent = axisModel.axis.getGridExtent();
const gapSignal = extent[0] > extent[1] ? -1 : 1;
Review comment:
Sorry for this late reply
The reason why `splitline` of two axis cannot align in most case, it's because axis will fill the `grid`[see here](https://github.com/apache/incubator-echarts/blob/next/src/coord/cartesian/Grid.ts#L166). `axis` will be split evenly by using `grid extent`. What I trying to do is calculate the `proportion` of axis in this grid, each axis has same standard which is `unionExtent`.
##########
File path: src/component/axis/AxisBuilder.ts
##########
@@ -372,7 +372,7 @@ const builders: Record<'axisLine' | 'axisTickLabel' | 'axisName', AxisElementsBu
const textStyleModel = axisModel.getModel('nameTextStyle');
const gap = axisModel.get('nameGap') || 0;
- const extent = axisModel.axis.getExtent();
+ const extent = axisModel.axis.getGridExtent();
const gapSignal = extent[0] > extent[1] ? -1 : 1;
Review comment:
@pissang That's correct, because without `min` `max`, axis will fill the `grid`, the proportion will be `100%`, with `min` or `max`, it will be less than `100%`(in most case, some case can still have `100%`, `max % interval === 0` ).
----------------------------------------------------------------
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.
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] [incubator-echarts] susiwen8 commented on a change in pull request #13210: Feat: multiple value axis alignment
Posted by GitBox <gi...@apache.org>.
susiwen8 commented on a change in pull request #13210:
URL: https://github.com/apache/incubator-echarts/pull/13210#discussion_r492437140
##########
File path: src/component/axis/AxisBuilder.ts
##########
@@ -372,7 +372,7 @@ const builders: Record<'axisLine' | 'axisTickLabel' | 'axisName', AxisElementsBu
const textStyleModel = axisModel.getModel('nameTextStyle');
const gap = axisModel.get('nameGap') || 0;
- const extent = axisModel.axis.getExtent();
+ const extent = axisModel.axis.getGridExtent();
const gapSignal = extent[0] > extent[1] ? -1 : 1;
Review comment:
@pissang So, if user specified `min` or `max`, `axis extent` will be different from `grid extent`.
----------------------------------------------------------------
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.
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] [incubator-echarts] echarts-bot[bot] commented on pull request #13210: Feat: multiple value axis alignment
Posted by GitBox <gi...@apache.org>.
echarts-bot[bot] commented on pull request #13210:
URL: https://github.com/apache/incubator-echarts/pull/13210#issuecomment-683751595
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/incubator-echarts/wiki/How-to-make-a-pull-request).
The pull request is marked to be `PR: author is committer` because you are a committer of this project.
----------------------------------------------------------------
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.
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] [incubator-echarts] pissang commented on a change in pull request #13210: Feat: multiple value axis alignment
Posted by GitBox <gi...@apache.org>.
pissang commented on a change in pull request #13210:
URL: https://github.com/apache/incubator-echarts/pull/13210#discussion_r492453250
##########
File path: src/component/axis/AxisBuilder.ts
##########
@@ -372,7 +372,7 @@ const builders: Record<'axisLine' | 'axisTickLabel' | 'axisName', AxisElementsBu
const textStyleModel = axisModel.getModel('nameTextStyle');
const gap = axisModel.get('nameGap') || 0;
- const extent = axisModel.axis.getExtent();
+ const extent = axisModel.axis.getGridExtent();
const gapSignal = extent[0] > extent[1] ? -1 : 1;
Review comment:
I thought the main reason two axes don't align is that they don't have the same split numbers. They should be aligned if the split numbers are same.
##########
File path: src/component/axis/AxisBuilder.ts
##########
@@ -372,7 +372,7 @@ const builders: Record<'axisLine' | 'axisTickLabel' | 'axisName', AxisElementsBu
const textStyleModel = axisModel.getModel('nameTextStyle');
const gap = axisModel.get('nameGap') || 0;
- const extent = axisModel.axis.getExtent();
+ const extent = axisModel.axis.getGridExtent();
const gapSignal = extent[0] > extent[1] ? -1 : 1;
Review comment:
<del>I thought the main reason two axes don't align is that they don't have the same split numbers. They should be aligned if the split numbers are same.</del>
I see, axis extent is different from grid extent only when `min`, `max` is fixed?
##########
File path: src/component/axis/AxisBuilder.ts
##########
@@ -372,7 +372,7 @@ const builders: Record<'axisLine' | 'axisTickLabel' | 'axisName', AxisElementsBu
const textStyleModel = axisModel.getModel('nameTextStyle');
const gap = axisModel.get('nameGap') || 0;
- const extent = axisModel.axis.getExtent();
+ const extent = axisModel.axis.getGridExtent();
const gapSignal = extent[0] > extent[1] ? -1 : 1;
Review comment:
<del>I thought the main reason two axes don't align is that they don't have the same split numbers. They should be aligned if the split numbers are same.</del>
I see, so axis extent is different from grid extent only when `min`, `max` is fixed?
----------------------------------------------------------------
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.
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] [incubator-echarts] susiwen8 commented on a change in pull request #13210: Feat: multiple value axis alignment
Posted by GitBox <gi...@apache.org>.
susiwen8 commented on a change in pull request #13210:
URL: https://github.com/apache/incubator-echarts/pull/13210#discussion_r494289507
##########
File path: src/coord/cartesian/Grid.ts
##########
@@ -85,18 +86,68 @@ class Grid implements CoordinateSystemMaster {
return this._rect;
}
+ calAxisNiceSplitNumber(axes: Axis2D[]) {
+ const splitNumber: number[] = [];
+ const niceAxisExtents: number[] = [];
+ each(axes, axis => {
+ niceScaleExtent(axis.scale, axis.model);
+ if (axis.type !== 'value') {
+ return;
+ }
+ const extent = axis.scale.getExtent();
+ const interval = (axis.scale as IntervalScale).getInterval();
+ splitNumber.push(
+ Math.floor((extent[1] - extent[0]) / interval)
+ );
+ const axisExtent = axis.getExtent();
+ niceAxisExtents.push((axisExtent[1] - axisExtent[0]) * interval / extent[1]);
+ });
+
+ return splitNumber;
+ }
+
+ resetAxisExtent(axes: Axis2D[], splitNumber: number[], maxSplitNumber: number): number[] {
+ const finalSplitNumber: number[] = [];
+ each(axes, function (axis, index) {
+ if (axis.type !== 'value') {
+ return;
+ }
+
+ if (!(maxSplitNumber - splitNumber[index])) {
+ finalSplitNumber.push(maxSplitNumber);
+ return;
+ };
+
+ const extent = axis.scale.getExtent();
+ const interval = (axis.scale as IntervalScale).getInterval();
+ if (((extent[1] - extent[0]) % interval) === 0) {
+ extent[1] = extent[1] + (maxSplitNumber - splitNumber[index]) * interval;
+ niceScaleExtent(axis.scale, axis.model, extent, maxSplitNumber);
+ }
+ else {
+ (axis.scale as IntervalScale).niceTicks(
+ maxSplitNumber + 1, null, null,
+ ((extent[1] - extent[0]) - ((extent[1] - extent[0]) % maxSplitNumber)) / maxSplitNumber
+ );
+ }
+ const finalScaleExtent = axis.scale.getExtent();
+ const finalInterval = (axis.scale as IntervalScale).getInterval();
+ finalSplitNumber.push(Math.ceil((finalScaleExtent[1] - finalScaleExtent[0]) / finalInterval));
+ });
+
+ return finalSplitNumber;
+ }
+
update(ecModel: GlobalModel, api: ExtensionAPI): void {
const axesMap = this._axesMap;
this._updateScale(ecModel, this.model);
- each(axesMap.x, function (xAxis) {
- niceScaleExtent(xAxis.scale, xAxis.model);
- });
- each(axesMap.y, function (yAxis) {
- niceScaleExtent(yAxis.scale, yAxis.model);
- });
+ const niceSplitNumX = this.calAxisNiceSplitNumber(axesMap.x);
+ const niceSplitNumY = this.calAxisNiceSplitNumber(axesMap.y);
+ const finalSplitNumberY = this.resetAxisExtent(axesMap.y, niceSplitNumY, Math.max(...niceSplitNumY));
+ const finalSplitNumberX = this.resetAxisExtent(axesMap.x, niceSplitNumX, Math.max(...niceSplitNumX));
Review comment:
Yes, it will change option
I will look into 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.
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