-
Notifications
You must be signed in to change notification settings - Fork 128
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
Add flag to CompareWorkspaces
so users can specify NaN == NaN
behavior
#38075
Conversation
e6cf09b
to
06b3996
Compare
9ac6b9f
to
eae42b0
Compare
048485b
to
90c03ff
Compare
db42c29
to
352eea1
Compare
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.
Fairly minor change requests. I like the centralized method and making whether nan should be equal to nan is an excellent thing to make explicit.
Later on, we should consider modernizing TableColumn
.
@@ -64,10 +65,10 @@ template <class Type> class TableColumn : public API::Column { | |||
std::string name = std::string(typeid(Type).name()); | |||
if ((name.find('i') != std::string::npos) || (name.find('l') != std::string::npos) || | |||
(name.find('x') != std::string::npos)) { | |||
if (length == 4) { |
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.
There is a lot of work we could do to make TableColumn not so ugly. Example: move implementation into the cpp file. That is a job for a different PR.
Description of work
Summary of work
Adds a flag to set whether
NaN
values should be considered equal to theCompareWorkspaces
algorithm.Purpose of work
When
CompareWorkspaces
was originally written, it was written so that if two workspaces hadNaN
for a value, the twoNaN
values would compare as equal. This is contrary to the IEEE 745 specification, which says thatNaN
compares false against everything, including otherNaN
s, so thatNaN == NaN
should evaluate as false.In attempting to fix this, it was found there are at least a few tests relying on the old behavior of comparing
NaN
s. Therefore, this provides a flag toCompareWorkspaces
so that users can elect forNaN
s to be considered equal.See also Issue #38088
Related to ORNL EWM 7196
(no further details at that link, only linking for bookkeeping)
Further detail of work
Within
CompareWorkspaces
, the methods to check if two values are within a tolerance have been replaced to rely on theFloatingPointComparison
operators, with a check to returntrue
if both values areNaN
and theNaNsEqual
flag was set totrue
.The
CheckAllData
flag will now work on two type of peaks workspaces.More tests were added for
CompareWorkspaces
andFloatingPointComparison
, to handle more cases involvingNaN
s or extreme numbers.A few cppcheck suppressions were removed.
Also, some comments were added in some parts of the code. These places were found using a search for
abs(
andfabs(
within the code for possible replacements withwithinAbsoluteDifference
. While not suitable for replacements, they could lead to improvements in the code.It was necessary to fix several system tests, which relied on previous behavior for
NaN
comparison. When theNaN
s were only in the uncertainties, then checking uncertainty was turned off. Otherwise, withNaN
s in they
-values. theNaNsEqual
flag was set for the comparison.The following tests were updated to work with new
NaN
behavior:The following three tests relied on comparing
NaN
s to finite floating points, which used to evaluate as true. There is no way to fix these without fixing the reference files. They are marked to skip, and in their previous forms were not meaningful. It is possible replacing theNaN
s with0
could fix some of them.See also issue #38088
To test:
New tests were added to
CompareWorkspacesTest.h
TableColumnTest.h
FloatPointComparisonTest.h
covering several situations with
NaN
s and also with extreme values.These tests should be convincing that the behavior of
NaN
s with and without the flag is as expected.Reviewer
Please comment on the points listed below (full description).
Your comments will be used as part of the gatekeeper process, so please comment clearly on what you have checked during your review. If changes are made to the PR during the review process then your final comment will be the most important for gatekeepers. In this comment you should make it clear why any earlier review is still valid, or confirm that all requested changes have been addressed.
Code Review
Functional Tests
Does everything look good? Mark the review as Approve. A member of
@mantidproject/gatekeepers
will take care of it.Gatekeeper
If you need to request changes to a PR then please add a comment and set the review status to "Request changes". This will stop the PR from showing up in the list for other gatekeepers.