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.

Sign up for GitHub

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

Jump to bottom

Fix #522: More robust rotation checking in egs++ transformations #525

Merged
merged 1 commit into from
Jan 15, 2020
Merged

Fix #522: More robust rotation checking in egs++ transformations #525

merged 1 commit into from
Jan 15, 2020

Conversation

jw3126
Copy link
Contributor

@jw3126 jw3126 commented Mar 29, 2019

No description provided.

@rtownson rtownson self-assigned this Mar 29, 2019
@rtownson rtownson changed the title More robust roation checking (#522) Fix #522: More robust rotation checking in egs++ transformations Mar 29, 2019
@blakewalters
Copy link
Contributor

Please provide a more descriptive comment for this pull request, @jw3126, perhaps including why this fix is necessary.

@rtownson
Copy link
Collaborator

rtownson commented May 6, 2019

@blakewalters there's more description in the issue, #522. Essentially the check against the identity matrix could fail due to floating point errors.

@jw3126
Copy link
Contributor Author

jw3126 commented May 7, 2019

@blakewalters You want me to expand the commit message?

@blakewalters
Copy link
Contributor

@jw3126: Maybe the commit message just needs a reference to issue #522 (e.g. "Resolves issue #522").

@jw3126
Copy link
Contributor Author

jw3126 commented May 12, 2019

@blakewalters More robust roation checking (Resolves issue #522) ok with that?

@blakewalters
Copy link
Contributor

Perfect, @jw3126. Thanks!

@ftessier
Copy link
Member

Our commit title format for fixing issues is Fix #nnn: (...); see https://github.com/nrc-cnrc/EGSnrc/wiki/Git-commit-messages. I will update and push to @jw3126, in view of merging this in develop.

@ftessier ftessier force-pushed the isrotation branch 2 times, most recently from 6b1ade5 to 3ed5116 Compare November 22, 2019 17:06
@ftessier
Copy link
Member

Adjusted the commit message, and updated the logic to test component-by-component. This is more verbose, but it is clearer and on average more efficient, arguably!

ftessier added a commit that referenced this pull request Nov 22, 2019
@ftessier
Reduce machine epsilon and add a distance epsilon
021b260 
Reduce the value of the egs++ floating-point epsilon from 10e-10 to
a value that is 4 times (2 bits away) from the true machine epsilon, and
appropriately for single and double precision versions of the code.

Add a distanceEpsilon constant that is independent of the machine
epsilon, and much larger, but still entirely sufficient for physical
dimensions (well below atomic dimensions in double precision). By
default, the base geometry sets its boundaryTolerance to this
constant. Its value is close to the previous value of epsilon (1e-10),
which should curb any change in floating point error triggers in the
geometry library at this point.

Earlier in #177, the epsilon constant was introduced to remove
hard-coded floating point comparison guards. The 1e-10 value was chosen
because it matched most of the hard-coded values in the geometry
library. However, this epsilon is now starting to be used as a general
float guard (see #525 for example), where 1e-10 is not appropriate, as
it effectively reduces the floating-point precision of the computation.
Hence the need for independent epsilon values for computations and for
geometrical purposes.
@ftessier ftessier mentioned this pull request Nov 22, 2019
Merged
jw3126
jw3126 commented Nov 25, 2019
View reviewed changes
Copy link
Contributor Author

@jw3126 jw3126 left a comment

Choose a reason for hiding this comment

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

Looks clean and correct to me.

@ftessier ftessier added this to the Release 2020 milestone Jan 13, 2020
@ftessier
Copy link
Member

Cleaned up the branch and adjusted commit messages. Keep this as two separate commits on side branch when merging in, to preserve the original commit by @jw3126.

mainegra
mainegra approved these changes Jan 14, 2020
View reviewed changes
Copy link
Contributor

@mainegra mainegra left a comment

Choose a reason for hiding this comment

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

Adding a bit more info regarding this pull request:

Rotation matrices are square matrices, with real entries. More specifically, they can be characterized as orthogonal matrices with determinant 1; that is, a square matrix R is a rotation matrix if and only if RT = R−1 and det R = 1.

From Wikipedia's definition of a rotation matrix.

@ftessier
Copy link
Member

ftessier commented Jan 15, 2020 •
edited
Loading

The comparisons with 0 and 1 in the original code are fine because those numbers have exact binary floating-point representations. But in general the (arbitrary) input numbers will carry roundoff errors, so it's worth adding epsilon guards. For the record, @jw3126, I think your error message in #522 arises because the determinant of your sample matrix is 1.0000006188980002 and trips the first float guard in isRotation().

@jw3126 @ftessier
Fix nrc-cnrc#522: float-guard the identity matrix test
9551fbc 
Provide floating-point error margins in testing whether or not a matrix
is the identity matrix. This is done on a component by component basis.
Previously, matrix components were compared with exactly '0' and '1', so
the test could fail on account of roundoff errors on the components.
@ftessier ftessier merged commit 011c307 into nrc-cnrc:develop Jan 15, 2020
@ftessier ftessier mentioned this pull request Jan 15, 2020
Closed
ftessier added a commit that referenced this pull request Jan 31, 2020
@ftessier
Reduce machine epsilon and add a distance epsilon
5faff9c 
Reduce the value of the egs++ floating-point epsilon from 10e-10 to
a value that is 4 times (2 bits away) from the true machine epsilon, and
appropriately for single and double precision versions of the code.

Add a distanceEpsilon constant that is independent of the machine
epsilon, and much larger, but still entirely sufficient for physical
dimensions (well below atomic dimensions in double precision). By
default, the base geometry sets its boundaryTolerance to this
constant. Its value is close to the previous value of epsilon (1e-10),
which should curb any change in floating point error triggers in the
geometry library at this point.

Earlier in #177, the epsilon constant was introduced to remove
hard-coded floating point comparison guards. The 1e-10 value was chosen
because it matched most of the hard-coded values in the geometry
library. However, this epsilon is now starting to be used as a general
float guard (see #525 for example), where 1e-10 is not appropriate, as
it effectively reduces the floating-point precision of the computation.
Hence the need for independent epsilon values for computations and for
geometrical purposes.
ftessier added a commit that referenced this pull request Aug 24, 2020
@ftessier
Reduce machine epsilon and add a distance epsilon
a0924cc 
Reduce the value of the egs++ floating-point epsilon from 10e-10 to
a value that is 4 times (2 bits away) from the true machine epsilon, and
appropriately for single and double precision versions of the code.

Add a distanceEpsilon constant that is independent of the machine
epsilon, and much larger, but still entirely sufficient for physical
dimensions (well below atomic dimensions in double precision). By
default, the base geometry sets its boundaryTolerance to this
constant. Its value is close to the previous value of epsilon (1e-10),
which should curb any change in floating point error triggers in the
geometry library at this point.

Earlier in #177, the epsilon constant was introduced to remove
hard-coded floating point comparison guards. The 1e-10 value was chosen
because it matched most of the hard-coded values in the geometry
library. However, this epsilon is now starting to be used as a general
float guard (see #525 for example), where 1e-10 is not appropriate, as
it effectively reduces the floating-point precision of the computation.
Hence the need for independent epsilon values for computations and for
geometrical purposes.
ftessier added a commit that referenced this pull request Aug 24, 2020
@ftessier
Reduce machine epsilon and add a distance epsilon
cbd60c5 
Reduce the value of the egs++ floating-point epsilon from 10e-10 to
a value that is 4 times (2 bits away) from the true machine epsilon, and
appropriately for single and double precision versions of the code.

Add a distanceEpsilon constant that is independent of the machine
epsilon, and much larger, but still entirely sufficient for physical
dimensions (well below atomic dimensions in double precision). By
default, the base geometry sets its boundaryTolerance to this
constant. Its value is close to the previous value of epsilon (1e-10),
which should curb any change in floating point error triggers in the
geometry library at this point.

Earlier in #177, the epsilon constant was introduced to remove
hard-coded floating point comparison guards. The 1e-10 value was chosen
because it matched most of the hard-coded values in the geometry
library. However, this epsilon is now starting to be used as a general
float guard (see #525 for example), where 1e-10 is not appropriate, as
it effectively reduces the floating-point precision of the computation.
Hence the need for independent epsilon values for computations and for
geometrical purposes.
ftessier added a commit that referenced this pull request Mar 24, 2021
@ftessier
Reduce machine epsilon and add a distance epsilon
4fb3273 
Reduce the value of the egs++ floating-point epsilon from 10e-10 to
a value that is 4 times (2 bits away) from the true machine epsilon, and
appropriately for single and double precision versions of the code.

Add a distanceEpsilon constant that is independent of the machine
epsilon, and much larger, but still entirely sufficient for physical
dimensions (well below atomic dimensions in double precision). By
default, the base geometry sets its boundaryTolerance to this
constant. Its value is close to the previous value of epsilon (1e-10),
which should curb any change in floating point error triggers in the
geometry library at this point.

Earlier in #177, the epsilon constant was introduced to remove
hard-coded floating point comparison guards. The 1e-10 value was chosen
because it matched most of the hard-coded values in the geometry
library. However, this epsilon is now starting to be used as a general
float guard (see #525 for example), where 1e-10 is not appropriate, as
it effectively reduces the floating-point precision of the computation.
Hence the need for independent epsilon values for computations and for
geometrical purposes.
ftessier added a commit that referenced this pull request Mar 24, 2021
@ftessier
Reduce machine epsilon and add a distance epsilon
fd008c2 
Reduce the value of the egs++ floating-point epsilon from 10e-10 to
a value that is 4 times (2 bits away) from the true machine epsilon, and
appropriately for single and double precision versions of the code.

Add a distanceEpsilon constant that is independent of the machine
epsilon, and much larger, but still entirely sufficient for physical
dimensions (well below atomic dimensions in double precision). By
default, the base geometry sets its boundaryTolerance to this
constant. Its value is close to the previous value of epsilon (1e-10),
which should curb any change in floating point error triggers in the
geometry library at this point.

Earlier in #177, the epsilon constant was introduced to remove
hard-coded floating point comparison guards. The 1e-10 value was chosen
because it matched most of the hard-coded values in the geometry
library. However, this epsilon is now starting to be used as a general
float guard (see #525 for example), where 1e-10 is not appropriate, as
it effectively reduces the floating-point precision of the computation.
Hence the need for independent epsilon values for computations and for
geometrical purposes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ftessier ftessier ftessier approved these changes

@mainegra mainegra mainegra approved these changes

@rtownson rtownson rtownson approved these changes

@blakewalters blakewalters blakewalters approved these changes

Assignees

@rtownson rtownson

Labels
improvement
Projects
None yet
Milestone
Release 2020
Development

Successfully merging this pull request may close these issues.
5 participants
@jw3126 @blakewalters @rtownson @ftessier @mainegra