Automatically adjust stepRatioScale to fix weired behaviour when calculating yAxis-domain #4453

ForestLinSen · 2024-04-24T20:17:47Z

Description

This PR is for an issue where minor changes in data could affect the Y-axis domain significantly(an example mentioned in the issue #4020 here). Various parameters impact this, but a relatively straightforward solution, after quite a few experiments, is to adjust the stepRatioScale.

This value was previously hardcoded to 0.05, it now can automatically adjust to make sure the calculated step is within the reasonable range

Related Issue

Weired behaviour when calculating yAxis domain: ["auto", "auto"]

How Has This Been Tested?

Locally tested and unit test

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My change requires a change to the documentation.
I have updated the documentation accordingly.
I have added tests to cover my changes.
I have added a storybook story or extended an existing story to show my changes

codecov · 2024-04-24T20:42:17Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.17%. Comparing base (ddba2c5) to head (4f8927b).
Report is 17 commits behind head on 3.x.

❗ Current head 4f8927b differs from pull request most recent head 935ad6a. Consider uploading reports for the commit 935ad6a to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##              3.x    #4453   +/-   ##
=======================================
  Coverage   95.17%   95.17%           
=======================================
  Files         111      111           
  Lines       21299    21307    +8     
  Branches     2932     2934    +2     
=======================================
+ Hits        20272    20280    +8     
  Misses       1021     1021           
  Partials        6        6

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ckifer · 2024-04-24T20:51:47Z

src/chart/generateCategoricalChart.tsx

@@ -802,6 +803,7 @@ export interface CategoricalChartProps {
  data?: any[];
  layout?: LayoutType;
  stackOffset?: StackOffsetType;
+  stepRatioControl?: StepRatioControl;


one idea I had was, instead of making this a separate parameter that gets passed to every chart at the highest level why don't we associate it with the domain (since thats where it is used). For example, we can keep everything as-is but allow an object as part of domain specification.

domain={'auto'}

OR

domain={[0, 10000]}

OR

`domain={(min, max) => [min, max]}

OR

domain={{ domain: 'auto' , settings: stepRatioControl: 0.1 }}
(or maybe somehow only allow it if domain is auto so there is no confusion)

Or if not the above then

domainSettings={{ stepRatioControl: 0.1 }}

on the X/YAxis?

This way there is less ambiguity against what the parameter affects/changes. Otherwise it seems arbitrary and hard to follow like the discussion in the other thread...

yeah I think combining this parameter with something is a good idea, but my concern is that it would require a lot of changes to the code that currently assumes the input value is an array containing two numeric values when the domain is set to auto - changing it to an object would add a lot messy. Another concern is that making it an object allows users to set any number, which might be problematic if the value is being set to too high/ low, without the enum type they would need an even better understanding of how to change the value. Anyway I like the idea, just wondering if there's a better way to combine this parameter

PavelVanecek · 2024-04-24T22:03:24Z

I would like to do something similar to what the d3 scales are doing, or what ggplot2 is doing. Instead of different parameters they accept whole different function and then call it. That way we keep the API simple and flexible. Domain already has a support for it, so we can export various domain functions and allow people swapping between them.

…

On Thu, Apr 25, 2024, 06:41 Sen Lin ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/chart/generateCategoricalChart.tsx <#4453 (comment)>: > @@ -802,6 +803,7 @@ export interface CategoricalChartProps { data?: any[]; layout?: LayoutType; stackOffset?: StackOffsetType; + stepRatioControl?: StepRatioControl; yeah I think combining this parameter with something is a good idea, but my concern is that it would require a lot of changes to the code that currently assumes the input value is an array containing two numeric values when the domain is set to auto - changing it to an object would add a lot messy. Another concern is that making it an object allows users to set any number, which might be problematic if the value is being set to too high/ low, without the enum type they would need an even better understanding of how to change the value. Anyway I like the idea, just wondering if there's a better way to combine this parameter — Reply to this email directly, view it on GitHub <#4453 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAIMTCVDYRU6URO43KXQA3LY7ARKNAVCNFSM6AAAAABGXS5KEWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDAMRQHE4DSMBZHA> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

ckifer · 2024-04-24T22:04:53Z

That would work for me, but we still have to expect a certain result to be returned and do something with it

ForestLinSen · 2024-04-24T22:46:42Z

Got an idea: if we don't care too much about customization and just want to fix the bug, I could change the algorithm logic to check if the calculated step is within a reasonable range, if not the stepRatioScale should automatically minus 0.01 everytime until it satisfy the requirement. What do you guys think?

ckifer · 2024-04-24T22:49:04Z

I'd be fine with that. What's the definition of a reasonable range?

ForestLinSen · 2024-04-24T22:52:45Z

I'd be fine with that. What's the definition of a reasonable range?

like if calculatedStep * 0.6 > (max-min)/tickCount then it's beyond the reasonable rage

ckifer · 2024-04-24T23:29:23Z

can you give an example?

If had the domain from the original issue #4020
what gets plugged into this formula and how does it help?

let data2 = [
  {
    date: "aaa",
    added: 3001,
    removed: -1000,
  },
  {
    date: "bbb",
    added: 2000,
    removed: -1000,
  },
  {
    date: "ccc",
    added: 1000,
    removed: -2001,
  },
];

min = -2001, max = 3001

ForestLinSen · 2024-04-25T17:09:50Z

can you give an example?

If had the domain from the original issue #4020 what gets plugged into this formula and how does it help?
let data2 = [
  {
    date: "aaa",
    added: 3001,
    removed: -1000,
  },
  {
    date: "bbb",
    added: 2000,
    removed: -1000,
  },
  {
    date: "ccc",
    added: 1000,
    removed: -2001,
  },
];
min = -2001, max = 3001

so here max - min = 5001, giving the roughStep 5001/4 = 1250.25. The initially calculated step when a stepRatioScale of 0.05 is 2000, then we have 2000 * 0.7 > 1250.25, it should reduce the thestepRatioScale by 0.01 until it satisfies the requirement. The result after adding this check looks like this:

ckifer · 2024-04-26T18:12:51Z

2000 * 0.7 > 1250.25

where does 0.7 come from?

ForestLinSen · 2024-04-26T18:24:15Z

2000 * 0.7 > 1250.25

where does 0.7 come from?

It's just the parameter we can choose to keep the calculated result within the reasonable range, ex. if it's 0.1 basically it has no effect at all because calculated result couldn't reach that high, if it's 0.9 the check mechanism could be too sensitive

ckifer · 2024-04-26T20:36:41Z

Ok I finally took a second to step through the current code and understand it much better. This makes a lot of sense to me, I think for "auto" we should keep "auto" functionality subjective and within recharts.

Lets try your idea out! And maybe we can add more comments as to what these functions are doing while we're at it. The Decimal library makes things a little hard to read

ckifer · 2024-04-26T20:38:36Z

Alternatively, does directly using D3 help us at all?
https://d3js.org/d3-axis

(probably not without more re-work to depend on it)

This reverts commit 90f4f15.

ForestLinSen · 2024-04-27T01:08:28Z

I have updated the code - should be a smaller change now

ckifer · 2024-04-27T03:13:35Z

Will take a look when I can, thanks

ckifer

few things

ckifer · 2024-05-02T05:27:42Z

src/util/scale/getNiceTickValues.ts

-  const formatStep = amendStepRatio.mul(digitCountValue);
+  let formatStep: Decimal;
+
+  do {


this should be in a regular while loop I think? In a do-while 0.05 will always get decremented to 0.04 before the check

ckifer · 2024-05-02T05:28:26Z

src/util/scale/getNiceTickValues.ts

+    formatStep = amendStepRatio.mul(digitCountValue);
+
+    stepRatioScale -= 0.01;
+  } while (formatStep.mul(0.7).gt(roughStep) && stepRatioScale > 0.02);


can we put 0.7 into a constant and name it so we know what it is later? Its a magic number at this point

ForestLinSen · 2024-05-02T18:37:35Z

few things

good points, have updated the code and also added another two constants

ckifer

this looks good to me!

@PavelVanecek tagging one more time for a look. This will cause defaults to change, it will be breaking, but it won't be as confusing as adding the ability to configure youself. It just tries to be a little more forgiving when calculating tick value steps

Add stepRatioControl

90f4f15

ckifer reviewed Apr 24, 2024

View reviewed changes

ForestLinSen added 2 commits April 27, 2024 01:37

Revert "Add stepRatioControl"

ab43d59

This reverts commit 90f4f15.

Automatically adjust stepRatioScale

4f8927b

ForestLinSen changed the title ~~Add stepRatioControl to fix weired behaviour when calculating yAxis-domain~~ Automatically adjust stepRatioScale to fix weired behaviour when calculating yAxis-domain Apr 27, 2024

ckifer requested changes May 2, 2024

View reviewed changes

Add constants for getFormatStep

935ad6a

ckifer approved these changes May 2, 2024

View reviewed changes

ckifer requested a review from PavelVanecek May 2, 2024 19:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Automatically adjust stepRatioScale to fix weired behaviour when calculating yAxis-domain #4453

Automatically adjust stepRatioScale to fix weired behaviour when calculating yAxis-domain #4453

ForestLinSen commented Apr 24, 2024 •

edited

codecov bot commented Apr 24, 2024 •

edited

ckifer Apr 24, 2024

ForestLinSen Apr 24, 2024

PavelVanecek commented Apr 24, 2024 via email

ckifer commented Apr 24, 2024

ForestLinSen commented Apr 24, 2024

ckifer commented Apr 24, 2024

ForestLinSen commented Apr 24, 2024

ckifer commented Apr 24, 2024

ForestLinSen commented Apr 25, 2024

ckifer commented Apr 26, 2024

ForestLinSen commented Apr 26, 2024

ckifer commented Apr 26, 2024

ckifer commented Apr 26, 2024 •

edited

ForestLinSen commented Apr 27, 2024

ckifer commented Apr 27, 2024

ckifer left a comment

ckifer May 2, 2024

ckifer May 2, 2024

ForestLinSen commented May 2, 2024

ckifer left a comment

Automatically adjust stepRatioScale to fix weired behaviour when calculating yAxis-domain #4453

Are you sure you want to change the base?

Automatically adjust stepRatioScale to fix weired behaviour when calculating yAxis-domain #4453

Conversation

ForestLinSen commented Apr 24, 2024 • edited

Description

Related Issue

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

codecov bot commented Apr 24, 2024 • edited

Codecov Report

ckifer Apr 24, 2024

Choose a reason for hiding this comment

ForestLinSen Apr 24, 2024

Choose a reason for hiding this comment

PavelVanecek commented Apr 24, 2024 via email

ckifer commented Apr 24, 2024

ForestLinSen commented Apr 24, 2024

ckifer commented Apr 24, 2024

ForestLinSen commented Apr 24, 2024

ckifer commented Apr 24, 2024

ForestLinSen commented Apr 25, 2024

ckifer commented Apr 26, 2024

ForestLinSen commented Apr 26, 2024

ckifer commented Apr 26, 2024

ckifer commented Apr 26, 2024 • edited

ForestLinSen commented Apr 27, 2024

ckifer commented Apr 27, 2024

ckifer left a comment

Choose a reason for hiding this comment

ckifer May 2, 2024

Choose a reason for hiding this comment

ckifer May 2, 2024

Choose a reason for hiding this comment

ForestLinSen commented May 2, 2024

ckifer left a comment

Choose a reason for hiding this comment

ForestLinSen commented Apr 24, 2024 •

edited

codecov bot commented Apr 24, 2024 •

edited

ckifer commented Apr 26, 2024 •

edited