Skip to content

fix double.roundToPrecision#35

Open
UliPlabst wants to merge 2 commits into
microsoft:mainfrom
UliPlabst:fix_doubleRoundToPrecision
Open

fix double.roundToPrecision#35
UliPlabst wants to merge 2 commits into
microsoft:mainfrom
UliPlabst:fix_doubleRoundToPrecision

Conversation

@UliPlabst

Copy link
Copy Markdown

This fixes #34. The issue turned out to be not the rounding itself but the calling of detectPrecision in a wrong way. Let's look at the implementation of detectPrecision:

export function detectPrecision(precision: number, x: number, y?: number): number {
if (precision !== undefined) {
return precision;
}
let calculatedPrecision;
if (!y) {
calculatedPrecision = getPrecision(x);
}
else if (!x) {
calculatedPrecision = getPrecision(y);
}
else {
calculatedPrecision = getPrecision(Math.min(Math.abs(x), Math.abs(y)));
}
return calculatedPrecision || DEFAULT_PRECISION;
}

As I understand if it get's passed a precision that is not null it just returns it. If precision is null then it detects the precision based on the boundary values x and y. If the precision is null it returns DEFAULT_PRECISION.
So the call

precision = detectPrecision(precision, DEFAULT_PRECISION);

in double.roundToPrecision does not make sense. As I understand it should pass x instead of DEFAULT_PRECISION.
I also added some more test cases to verify my problems were fixed. All tests pass for me.

@helen508light

Copy link
Copy Markdown

Have added task with this PR to our typeutils tasks backlog. It would be reviewed soon

@helen508light

Copy link
Copy Markdown

Also @UliPlabst please update your changes, now they are out-of-date

@UliPlabst

Copy link
Copy Markdown
Author

@helen508light I updated the branch. Can you merge it?

@helen508light

Copy link
Copy Markdown

@UliPlabst
I've created task for this changes and it will be reviewed by team, but there is not ETA

@UliPlabst

Copy link
Copy Markdown
Author

okay thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Double.roundToPrecision rounding issue

3 participants