Skip to content
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

Fix machine epsilon for double #4096

Merged
merged 6 commits into from Jun 13, 2020
Merged

Fix machine epsilon for double #4096

merged 6 commits into from Jun 13, 2020

Conversation

rstm-sf
Copy link
Contributor

@rstm-sf rstm-sf commented Jun 9, 2020

What does the pull request do?

Fixing inaccuracy in machine epsilon found by @MarchingCube #4092 (comment)

What is the current behavior?

The result of methods from the MathUtilities class using machine epsilon was not always correct.

What is the updated/expected behavior with this PR?

The result of methods from the MathUtilities class using machine epsilon.

Checklist

@danwalmsley
Copy link
Member

@rstm-sf is this something we should also upstream to dotnet? so that double.Epsilon is correct in the framework?
disclaimer I have no idea what this is ;)

@jmacato
Copy link
Member

jmacato commented Jun 10, 2020

@rstm-sf i think this PR could be expanded elsewhere in the whole repo since we use double.Epsilon alot.

@rstm-sf
Copy link
Contributor Author

rstm-sf commented Jun 10, 2020

@rstm-sf is this something we should also upstream to dotnet? so that double.Epsilon is correct in the framework?
disclaimer I have no idea what this is ;)

This good answer what is double.Epsilon ;)

@rstm-sf
Copy link
Contributor Author

rstm-sf commented Jun 10, 2020

There is another class similar to MathUtilities. He has the next epsilon:

internal const double DBL_EPSILON = 1e-6;

Which is better to choose?

@MarchingCube
Copy link
Contributor

There is another class similar to MathUtilities. He has the next epsilon:

internal const double DBL_EPSILON = 1e-6;

Which is better to choose?

Even WPF has like 3 copies of this class, each one with a different constant. Would need to figure what is the reason behind it.

@rstm-sf
Copy link
Contributor Author

rstm-sf commented Jun 10, 2020

Even WPF has like 3 copies of this class, each one with a different constant. Would need to figure what is the reason behind it.

It seems to me that so historically. I prefer the single-precision option, since the pixels are integer?

@MarchingCube
Copy link
Contributor

Even WPF has like 3 copies of this class, each one with a different constant. Would need to figure what is the reason behind it.

It seems to me that so historically. I prefer the single-precision option, since the pixels are integer?

Depends during which phase - during layout (and many of our calculation happen during this phase) pixels are floating point due to DPI scaling. So even if user is using integers values everything will get multiplied by a scaling factor. Because of that one must be quite careful since precision errors accumulate easily and one of the reasons why layout has own set of rounding functions.

@rstm-sf
Copy link
Contributor Author

rstm-sf commented Jun 10, 2020

Ok, now I began to think that the error for double precision is better

@rstm-sf
Copy link
Contributor Author

rstm-sf commented Jun 10, 2020

Maybe it’s worth just creating a methods with a float signature and a single precision check?

@MarchingCube
Copy link
Contributor

Problem is that almost all of our calculations are done using double precision (I am still not sure why WPF is using double and not single precision), so exposing single precision API will both force users to cast and potentially introduce truncation errors.

@rstm-sf
Copy link
Contributor Author

rstm-sf commented Jun 11, 2020

I mean, it’s better to add similar checks for single precision ;)

Copy link
Contributor

@MarchingCube MarchingCube left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@MarchingCube MarchingCube merged commit adb401a into AvaloniaUI:master Jun 13, 2020
@rstm-sf rstm-sf deleted the bugfix/double_precision branch June 22, 2020 04:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants