New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Automatically adjust stepRatioScale to fix weired behaviour when calculating yAxis-domain #4453
base: 3.x
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. |
@@ -802,6 +803,7 @@ export interface CategoricalChartProps { | |||
data?: any[]; | |||
layout?: LayoutType; | |||
stackOffset?: StackOffsetType; | |||
stepRatioControl?: StepRatioControl; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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: ***@***.***>
|
That would work for me, but we still have to expect a certain result to be returned and do something with it |
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 |
I'd be fine with that. What's the definition of a reasonable range? |
like if |
can you give an example? If had the domain from the original issue #4020
min = -2001, max = 3001 |
so here max - min = 5001, giving the roughStep 5001/4 = 1250.25. The initially calculated step when a |
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 |
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 |
Alternatively, does directly using D3 help us at all? (probably not without more re-work to depend on it) |
This reverts commit 90f4f15.
I have updated the code - should be a smaller change now |
Will take a look when I can, thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
few things
src/util/scale/getNiceTickValues.ts
Outdated
const formatStep = amendStepRatio.mul(digitCountValue); | ||
let formatStep: Decimal; | ||
|
||
do { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
src/util/scale/getNiceTickValues.ts
Outdated
formatStep = amendStepRatio.mul(digitCountValue); | ||
|
||
stepRatioScale -= 0.01; | ||
} while (formatStep.mul(0.7).gt(roughStep) && stepRatioScale > 0.02); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
good points, have updated the code and also added another two constants |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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
Checklist: