tcunit / TcUnit

An unit testing framework for Beckhoff's TwinCAT 3
Other
273 stars 75 forks source link

AssertArrayEquals should have REAL and LREAL versions #31

Closed DavidHopkinsFbr closed 5 years ago

DavidHopkinsFbr commented 5 years ago

AssertEquals_REAL and AssertEquals_LREAL accept an extra delta parameter for specifying acceptable precision for equality.

There currently don't seem to be any AssertArrayEquals methods for REAL and LREAL types, presumably because of the invalidity of doing naive equality comparisons for floating point types. But it would be nice if there were floating point array equality assertions which have the same delta parameter as the individual value equality asserts.

It seems like a fairly straightforward thing to implement at first glance. Would you like me to draft an implementation and submit a PR?

sagatowski commented 5 years ago

Sounds like a good idea. Make sure to read the https://github.com/tcunit/TcUnit/blob/master/CONTRIBUTING.md

Most importantly, make sure to add a few tests to properly test the new functionality to TcUnit-Verifier and check that you get the expected output. I think there are already some test suites in the TcUnit-Verifier software for arrays, and that would probably be a good place to put them. https://github.com/tcunit/TcUnit-Verifier

Looking forward to your code.

DavidHopkinsFbr commented 5 years ago

Ok. I might also throw in some Assert methods for 2d and 3d LREAL arrays, since I have use cases for those.

DavidHopkinsFbr commented 5 years ago

I'm using these arrays for doing matrix math. The results of the matrix operations often include values with a pretty wide range of exponents - some elements have exponents on the order of 10^1, 10^2 or 10^3 while other elements are close to zero with exponents like 10^-6 or 10^-8.

So it seems a bit questionable to have a single fixed-magnitude delta to accommodate floating-point imprecision, which is used for comparing all the matrix elements no matter how large the element value is.

I'm thinking about making the delta for AssertArray_REAL and AssertArray_LREAL be proportional to the magnitude of the expected value, rather than an absolute value. The assertion test would look like this:

ABS(Expected - Actual) > Expected * Delta

instead of this:

ABS(Expected - Actual) > Delta

What do you think?

sagatowski commented 5 years ago

I think that will limit the usage too much and be too specific. Also, all other unit testing frameworks I've been using have the first proposed solution as implementation. Compare for instance to JUnit (which has been the main inspiration, at least regarding the assertions): https://junit.org/junit4/javadoc/4.12/org/junit/Assert.html#assertArrayEquals(float[],%20float[],%20float)

sagatowski commented 5 years ago

To solve your particular problem I would just use the normal (non array) assertion and call it multiple times but set the Delta to (Expected * Delta) for every call to assert.

DavidHopkinsFbr commented 5 years ago

Ok, makes sense. I think having the framework behave in a familiar way to existing frameworks is certainly important. Still, I think that a tolerance algorithm which works well for different magnitude values in the same array would be a more generally usable tool than a fixed absolute tolerance for the entire array.

I note that the approach taken by xUnit is that the floating point numbers are rounded to a certain number of (decimal) significant figures, and then the rounded values are tested for equality.

That's a pretty intuitive way to specify the acceptable level of precision, but it's still insensitive to the magnitude of the values being compared. What do you think of that approach, in principle?

It could be very straightforwardly implemented using LREAL_TO_FMTSTR, although that wouldn't be an especially computationally efficient approach.

DavidHopkinsFbr commented 5 years ago

On closer inspection, xUnit and the LREAL_TO_FMTSTR approach both use "precision" to mean "decimal places/fractional digits" rather than "significant figures", so neither is actually independent of the magnitude of the Expected value.

Also, doing ABS(Expected - Actual) > Expected * Delta is degenerate when Expected = 0 - the tolerance disappears entirely!

sagatowski commented 5 years ago

I'm not sure it's a good idea to have a different implementation for the delta/precision of arrays compared to the non-arrays (so using delta for non-arrays and precision for arrays would probably be quite confusing). Again, looking at the source code for various frameworks, they all use the same approach for non-arrays as arrays.

DavidHopkinsFbr commented 5 years ago

Yep, no problem. Will do as suggested.

sagatowski commented 5 years ago

Hi David! How is progress on this issue? I'm planning to do the next release of TcUnit in the coming week and was hoping to have this addition included as well.

DavidHopkinsFbr commented 5 years ago

Submitted some code for you. I'm frankly inexperienced with Github (still an SVN shop at my employer!) so I hope I did it right.

sagatowski commented 5 years ago

Implemented in commit e61fa91744cd43c01afaf37958a6e58df07cac60, 795b9cd840f04d65fed976a5b32adedfc12b499c and 48d5135a1e820293722fe4a990404304454e87c7.