Page MenuHomePhabricator

Implement comparison functions for basic data-classes
Closed, ResolvedPublic

Description

In particular the testing of image-processing algorithms would profit from comparison functions for basic MITK types, in particulary the mitk::Image.

After beeing discussed in the MITK-Group Meeting, the implementation of such operators has to be

(1) non-member, non-friend methods
(2) declared in the header of the corresponding class header and implemented in the same .cpp file as the class implementation

Event Timeline

Have the method names been discussed somewhere? In my opinion "IsEqual" is not a good name, because it is a synonym for operator==. As there are too many ways to understand "equal" or "similar", I would go for more descriptive names (depending on what set of comparators you want to implement). I would suggest to write down a meaningful list of operators that you would like to see implemented within MITK, not only the first two ones.

E.g:

bool
HavePixelGridInCommon(

const Image*, 
const Image*,
bool sameOrigin = true,
bool sameExtent = true);

bool
HaveSamePixelValues(

const Image*, 
const Image*,
bool compareWorldPositions = true,
bool strictlySameType = false,
bool checkOnlyOverlap = true);

These examples are not complete, but I hope they show what I'd expect. Such a list could also be presented on the mailinglist (with request for comments).

New remote branch pushed: bug-11925-CompareFunctions

The status of the bug is as follows:
-We discussed in several meetings and our retreat, that we definitely need comparison methods for testing and other purposes.
-There are many ways how to implement these methods (e.g. == Operator, Filters, static methods etc.). For now, we chose a concept described in the core change request. Code examples can be found in: bug-11925-CompareFunctions.

Todo List:
-Implement methods for Image, PointSet and Surface
-Search in existing tests for "bad/old" comparison methods and replace them

Todo List:
-Implement methods for Image, PointSet and Surface
-Search in existing tests for "bad/old" comparison methods and replace them
-Talk to geometry experts if we need comparison methods for Geometry2D and other classes.

One remark related to the scope of the AreEqual() methods:

I order to keep the public API of MITK classes as minimal as possible, the AreEqual() methods should not be member methods. As far as I can see, they do not required non-public access (and actually shouldn't anyway) to the classes for which they test for equality.

One point similar to Saschas:

  • The wiki examples could be static functions and do not need to be part of the class

And one in addition:

  • regarding "AreOriginsEqual()" I would suggest a more general function "ArePointsEqual", which could not only be used for the combination of geometry+origin
    • looking more at this example, we already have a list of implementations in mitkVector.h. This file a good name "Equal()". We should keep that name to keep consistent
    • perhaps we can just create a file "mitkEqual.h", which may list all Equal() functions? The file could also host some of parts of mitkVector.h

To not keep the "?" state for too long, I'd vote for "-" now. Please consider our comments and talk to Sascha in person to come to a descision (I would attend a meeting if neccessay).

We will discuss details about the namespace in the next MITK meeting.

Regarding Daniels issues:
-Methods are already static
-Equal is already used

Todo List:
-Implement methods for Image, PointSet and Surface
-Search in existing tests for "bad/old" comparison methods and replace them
-Talk to geometry experts if we need comparison methods for Geometry2D and other classes.
-Implement mitkCompareImageFilter

This bug is now part of the Bugsquashing SCRUM program.

New remote branch pushed: bug-11925-CompareFunctions-MigrationBranch

Regarding the Equal() method for surfaces, we in turn need an Equal() method for vtkPolydatas which I will in turn implement in the mitkSurface.cpp

As vtkPolydatas are rather complex structures (Containing a list of points and four lists of cells, which only reference the index of points), we cannot in turn create an equal method for vtkCells, as these only know the indexes for the points, but not whether an index represents the same point.

E.g:
The cell
3 4 1 2 (read 3 points, No. 4 No. 1 and No. 2)
may not be the same as cell
3 4 1 2
but may be the same as cell
3 4 3 2

If the point lists are:
0 0 1 0 1 0 1 0 0 1 1 1 (read as point 1 { x = 0, y = 0, z = 1}, point 2 { x = 0, ...} ...)
and
1 0 0 0 1 0 0 0 1 1 1 1

As well as cell
3 4 1 2
not being the same as
3 2 1 4
but being the same as
3 1 2 4
(due to handedness)

For this reason I will refrain from creating a very complex Equal() function unless it actually is needed by someone.

Instead I will just check:

  1. Are the number of cells/vertices/lines/polys/stripes the same?
  2. Is every point of one point list present in the other point list (with the same coordinates within eps)?

During yesterdays bugsquashing we had a long discussion regarding implementation details, the results are as follows:

All methods will have the signature:
mitk::Equal( type* obj1, type* obj2, mitk::Scalartype eps = mitk::eps )

If equality fails there will always be an output to MITK_INFO giving enough information, that the problem can be debugged from the dashboard without having to run a debug build (especially, what was compared, what was obj 1, what was obj 2, why is it not equal)
e.g. something like
Comparing points - 3,4,1 - 1,2,3 - distance is larger than 0.000000001

All output of numbers should use std::setprecision(12)

Methods will be declared in the header of the class it compares, defined in the cpp of the corresponding class.
e.g.
mitk::Equal( mitk::Surface*, mitk::Surface*)
in
mitkSurface.h and mitkSurface.cpp

If we need an Equal method for a class which is not part of MITK and does not supply its own (such as most of the vtk classes) we will implement it as close to the vtk class as possible in MITK.

e.g.
mitk::Equal(vtkPolyData*, vtkPolyData*)
in mitkSurface.h and mitkSurface.cpp

One further thing was discussed on tentatively agreed upon, though it will most likely be brought up in next weeks MITK meeting:

Every class should have one and only one Equal() method. If there is a special, important context it is used in the context will mention why.

e.g.

mitk::Equal(points)
{
if(<are not equal>)
{

MITK_INFO << "Points not equal: " << point1 << " - " << point2 << " - " << "distance";

}
}

mitk::Equal(geometries)
{
if( !mitk::Equal(points) )
{

MITK_INFO << "Origins not equal";

}
}

instead of having a separate function to compare origins (as discussed in e.g. comment 6 ).

New remote branch pushed: bug-11925-CompareFunctions-MigrationBranch-MITK-Surface

Update:
1.Implement methods for
Image DONE -> mitkCompareImageFilter
PointSet DONE
Surface DONE
Geometry2D
TimeSlicedGeometry

  1. Add verbose flag to all equal methods for detailed console output.
  1. Implement MITK_TEST_EQUAL and MITK_TEST_NOT_EQUAL macros which takes 3 arguments (lefthandside, righthandside and a text description similar to MITK_TEST_CONDITION). This macro calls the mitk::Equal method and sets the verbose flag to true.

Future Tasks:
-Search in existing tests for "bad/old" comparison methods and replace them
-Talk to geometry experts if we need comparison methods for Geometry2D and other classes.
-Implement

[26c8bc]: Merge branch 'bug-11925-CompareFunctions-MigrationBranch'

Merged commits:

2013-08-30 17:22:05 Thomas Kilgus [4bdd7c]
Finished documentation and last minor fixes.


2013-08-29 17:02:55 Thomas Kilgus [6dc97e]
Docu and minor fixes.


2013-08-28 18:32:06 Thomas Kilgus [add5a2]
Added a simple test case for surface: Equal_DifferentPoints_ReturnsFalse().


2013-08-28 17:21:51 Thomas Kilgus [a950e0]
Finished work on Equal methods.

Introducing new mitkTesting.h with verbose version of the mitkVector equal methods. New Macros MITK_TEST_EQUAL and MITK_TEST_NOT_EQUAL with default parameters.
For scalar types, there are now two different Equal signatures:
mitk::Equal(scalar1, scalar2, eps=mitk::eps) with 2 or 3 parameters
mitk::Equal(scalar1, scalar2, eps, verbose) with exactly 4 paramters to get rid of ambiguous function calls.


2013-08-28 14:32:19 Thomas Kilgus [b4093c]
Minor test adaption.


2013-08-23 11:53:01 Caspar Goch [a9cb39]
Added output to equal methods for scalars, this adds an include to the mithVector.h


2013-08-22 16:43:55 Caspar Goch [6a56a6]
Merge branch 'bug-11925-CompareFunctions-MigrationBranch-MITK-Surface' into bug-11925-CompareFunctions-MigrationBranch


2013-08-22 16:38:08 Caspar Goch [56472a]
Added test for null input


2013-08-22 16:33:50 Caspar Goch [22a9fe]
Moved some code from setup to individual tests


2013-08-22 16:00:33 Caspar Goch [69ac53]
Exports added, test now tests something


2013-08-22 14:16:02 Caspar Goch [5912f3]
Point comparison implemented and output only on failure of equality


2013-08-22 13:32:54 Caspar Goch [e513e6]
Compiles, but commented out region equal, also points not yet compared


2013-08-22 12:56:31 Caspar Goch [45f406]
Make non const


2013-08-22 12:55:35 Caspar Goch [5d8e29]
Fixed most compile errors


2013-08-22 11:00:29 Caspar Goch [577e10]
WIP: Started adding Equal methods to mitk:surface and added a test which as of yet tests nothing


2013-08-21 18:47:47 Thomas Kilgus [32b60e]
Removed old "AreEqualTest". New version is "EqualTest".


2013-08-21 18:45:21 Thomas Kilgus [e58da8]
Merge branch 'bug-11925-CompareFunctions-PointSet' into bug-11925-CompareFunctions-MigrationBranch


2013-08-21 18:43:44 Thomas Kilgus [3325b8]
mitk::Equal for PointSets including a test. Minor fixes to Geometry3D.


2013-08-21 18:30:36 Jan Hering [c62dfa]
Fixing CompareImageFilter

  • also calling compare only if the images are equal so far

2013-08-21 18:09:53 Jan Hering [21d384]
Merge branch 'bug-11925-CompareFunctions-Image' into bug-11925-CompareFunctions-MigrationBranch


2013-08-21 18:09:09 Jan Hering [775143]
Renaming test and adapting to new API


2013-08-21 18:06:40 Jan Hering [b1d4ff]
Implementing Equal for mitk::Image

  • declaration out of class
  • removed previous AreEqual() methods

2013-08-21 17:53:15 Jan Hering [1fb8fd]
Removed automatic output


2013-08-21 16:56:11 Jan Hering [614b0b]
Adapting call to CompareFilter


2013-08-21 16:55:19 Jan Hering [ad5214]
Enabling CompareFilter in CMake


2013-08-21 16:54:26 Jan Hering [984c67]
Implementing Image compare filter

  • using the itk::Testing::ComparisonImageFilter

2013-08-21 16:15:55 Thomas Kilgus [04b715]
Merge branch 'bug-11925-CompareFunctions-Geometry3D' into bug-11925-CompareFunctions-MigrationBranch


2013-08-21 16:14:18 Thomas Kilgus [e7876c]
Redesign of AreEqual methods to Equal. Renamed test accordingly.


2013-08-21 14:26:37 Jan Hering [25b193]
Merge remote-tracking branch 'origin/bug-11925-CompareFunctions' into bug-11925-CompareFunctions-MigrationBranch

Conflicts:
Core/Code/Testing/mitkImageTest.cpp


2013-08-15 11:35:04 Thomas Kilgus [d678aa]
Work in progress: new test for comparing images, new functions for comparing images, made functions in geometry3d test static.


2013-08-14 13:17:56 Thomas Kilgus [b3e176]
Renamed test to Geometry3DAreEqual.


2013-07-17 18:04:58 Thomas Kilgus [c0fe84]
New smaller methods for comparion of Geometry3D including a unit test and docu.


2013-07-10 17:44:35 Thomas Kilgus [6ac499]
Enhanced the CompareGeometryTest, name changed to mitk::Geometry3D::AreEqual and mitk::Image::AreEqual.


2013-07-10 14:07:37 Thomas Kilgus [0e026e]
Added first test for compare functions.


2013-07-03 15:42:56 Thomas Kilgus [e66375]
Merge remote-tracking branch 'origin/bug-11925-Compare-for-base-classes'


2013-07-03 09:50:50 Jan Hering [bd9fb2]
[WIP] Compare filter for images


2013-07-03 09:49:02 Jan Hering [3f7754]
Merge branch 'bug-11925-Basic-comparison-operators' into bug-11925-Compare-for-base-classes

Conflicts:
Core/Code/DataManagement/mitkGeometry3D.cpp
Core/Code/Testing/mitkImageTest.cpp


2012-05-16 16:44:11 Jan Hering [bf840f]
Renaming AreIdentical to IsEqual

  • moving the comparison methods to a separate namespace 'compare'

2012-05-16 16:13:05 Jan Hering [d6f059]
Enhancing mitkImageTest

  • several test cases for AreIdentical() method with same and different
  • images

2012-05-16 16:12:25 Jan Hering [bc3169]
Added PixelType comparison to AreIdentical()


2012-05-16 15:15:05 Jan Hering [f974a7]
Making AreIdentical() more verbose


2012-05-16 14:55:44 Jan Hering [9a0551]
Added AreIdentical() method for mitk::Image


2012-05-16 14:54:57 Jan Hering [07eb8e]
Added AreIdentical() method for Geometry3D

[93dffa]: Merge branch 'bug-11925-CompareMethods-clang-fixes'

Merged commits:

2013-09-02 18:00:05 Jan Hering [6342ae]
Fixing parantheses in if expressions

  • clang outputs a warning (=error in Core) for following expressions: 'if(( x == y ))' and the double parentheses is obsolete

[c05762]: Merge branch 'bug-11925-Handedness'

Merged commits:

2013-09-03 11:01:35 Jan Hering [0cf803]
Consistent naming scheme in Equal methods

[accdf8]: Merge branch 'bug-11925-EqualMethods-EnahnceDocumentation'

Merged commits:

2013-09-04 14:02:55 Jan Hering [1a0f81]
Enhncing documentation of testing methods

  • new doxygen group MITKTestingAPI
  • adding all Equal methods into the new group

New remote branch pushed: bug-11925-EqualUpdateTests

[8ddfff]: Merge branch 'bug-11925-EqualMethods-EnahnceDocumentation'

Merged commits:

2013-09-05 11:53:25 Jan Hering [d7e9c7]
Enhancing testing documentation

  • correcting text
  • including also the remaining macros in mitkTestinMacros.h into group MITKTesting

Status, resp. what is missing? Should the bug remain opened?

Depends if we would like to re-use the core-change request, to use the new methods in old tests... If not, we can just close this here.