LD feature newtry

[ldeniau]

[second attempt to make a clean pull request, hope it will be fine]
As discussed with Philippe Fremy by email, I do a pull request for features I needed and proved to be extremely useful to test codes, including LuaJIT itself and scientific codes.

Modified:

• assertTrue/False to be strict (essential), user can still do lazy False (i.e. not x instead of x == false) or use the renamed version hereafter.
• Non-strict assertTrue/False have been renamed assertIf/assertNotIf after discussion with NiteHawk.
• Remove extra margin in assertAlmostEquals, as it prevents any serious use in numerical codes (its main purpose, right?). Moreover, it is not expected from user point of view to have it's own margin being modified in the back (and fails to check correctly for bugs...). EPSILON is used as default margin for common cases if none is provided. Removing the old behaviour extends the flexibility of assertAlmostEquals since it allows to check for absolute error vs expected, absolute error minus expected versus zero, relative error vs expected and relative error divided by expected versus one, and checked within the user defined margin (four common cases mostly used in numerical tests).

Added:

• assertIsNaN and variants (true check for numerical NaN).
• assertIsInf and variants (check for numerical ±Inf). For signed checks of infinity, assertEquals is the right tool. For signed checks of zero, assertEquals is also the right tool (i.e. check 1/x versus ±inf).
• -x, --exclude PATTERN, works as -p but filter out tests matching the pattern. It improves the selection flexibility as Lua patterns are not expressive to negate matching. This filter out comes after the -p filter if any: all-tests -> selected-tests -> selected-tests - excluded tests -> run-tests.
• -c, --count NUM to run NUM times the tests and ensure testing also the JITed version of the code (essential with LuaJIT). setUp and tearDown must be part of the loop to preserve referential transparency of unit tests. The failure message displays also the iteration number where an assertion failed if this option is specified.

Typical use of added extensions assuming that TestPerf are performance tests (longer to run):
./luajit testsuite.lua -x TestPerf # only regression tests
./luajit testsuite.lua -p TestPerf # only performance tests
./luajit testsuite.lua -c 4 -x TestPerf # test referential transparency of regression tests
./luajit testsuite.lua -c 100 -x TestPerf # test JITed version of regression tests

Last use is essential to check consistency between interpreted and compiled code with LuaJIT. I found few bugs and inconsistencies with this feature!

Do not hesitate to ask me information to clarify any points, especially for the documentation. I have motivated all my proposal through email discussions with Philippe and NiteHawk. As a general comment, the philosophy of the proposal is to improve strictness of the unit tests (never relax it!) and flexibility of the framework (in that order).

Best,
Laurent.

[n1tehawk]

Thank you, this looks much cleaner now!

Side note: This PR has replaced / superseded #66, while the previous discussion there is still relevant. (Readers might want to cross-check it to get the complete picture.)

[ldeniau]

On 26 Jul 2016, at 14:16, NiteHawk notifications@github.com wrote:

Thank you, this looks much cleaner now!

Very good! Doing my first pull request so lately in the night was not a good idea. I can also help to review the documentation if needed.
Side note: This PR has replaced / superseded #66 #66, while the previous discussion there is still relevant. (Readers might want to cross-check it to get the complete picture.)

Yes, I closed the previous PR, instead of deleting it to keep track of the discussion.

Best,
Laurent.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub #68 (comment), or mute the thread https://github.com/notifications/unsubscribe-auth/ABKFz9WaZLoYK7zosKZD4Al-q1-vnMw0ks5qZfqPgaJpZM4JVETw.

[bluebird75]

For the assertTrue / assertFalse, after some more thinking, I agree with Laurent.

I also had the case of incorrect catch of nil through assertFalse which prevented me to find a bug in my code. Strictness is a key feature of unit testing. So let's go with strict. I don't like the assertIf name for the old behavior; the name does not carry enough the functionality. assertNilOrFalse looks better as a name and carry the meaning correctly.

[bluebird75]

For merging the changes, I have my internal policy of merging functionality along with unit-test that validate it. This way, I can ensure that core luaunit is always as stable as possible.

So, either Laurent, you add more unit-tests for the functionality which you have added, or you leave it as it is and I will write the unit-test myself. I already appreciate the effort you have been putting into contributing to luaunit so I am hesitant to ask for more...

[n1tehawk]

Side note: If we aim to support multiple numeric precisions, it might become necessary to 'dynamically' select EPSILON depending on the expected magnitude of rounding errors - see e.g. n1tehawk@fc8ec20. A value that is suitable for Lua's default "double" precision may be useless (too small to compensate for errors) with "single" precision.

[ldeniau]

On 27 Jul 2016, at 09:03, NiteHawk notifications@github.com wrote:

Side note: If we aim to support multiple numeric precisions, it might become necessary to 'dynamically' select EPSILON depending on the expected magnitude of rounding errors - see e.g. n1tehawk/luaunit@fc8ec20 n1tehawk@fc8ec20. A value that is suitable for Lua's default "double" precision may be useless (too small to compensate for errors) with "single" precision.

Single precision is not implemented in Lua (needs special recompilation of DUALNUM). The correct double precision M.EPSILON should be 2^-52 ~ 2.221e-16 so the user must be able to set its default precision (assuming IEEE 754 representation). Moreover, values like 1e-7 or 1e-12 are more about uncertainties than precision. And 1e-7 from you commit is too small for single precision (FLT_EPSILON = 2^-23 ~ 1.1921e-7 > 1e-7).

Why not simply use:

single precision (if any): M.EPSILON = 2^4 * 2^-23 ~ 2e-6
double precision: M.EPSILON = 2^8 * 2^-52 ~ 6e-14

Larger epsilon may be not be strict enough to detect bugs, even in simple cases… If precision error reach this level of EPSILON, something wrong is going on and/or the test itself introduces enough uncertainty that the user must provide appropriate epsilon. For example:

print((60-math.deg(math.pi/3))/2^-52) —> 32

Tells you that despite of its simplicity, radian to degree conversion is not accurate for 60 degrees (i.e. pi/3) and needs a margin of 32 * 2^-52 = 32 * DBL_EPSILON… Hiding this fact is not good for users, it is often better to give an opportunity to the users to understand what’s happening (and maybe learn something useful).

Best,
Laurent.

[ldeniau]

Hi,

On 27 Jul 2016, at 09:46, Laurent Deniau laurent.deniau@gmail.com wrote:

On 27 Jul 2016, at 09:03, NiteHawk <notifications@github.com mailto:notifications@github.com> wrote:

Side note: If we aim to support multiple numeric precisions, it might become necessary to 'dynamically' select EPSILON depending on the expected magnitude of rounding errors - see e.g. n1tehawk/luaunit@fc8ec20 n1tehawk@fc8ec20. A value that is suitable for Lua's default "double" precision may be useless (too small to compensate for errors) with "single" precision.

Single precision is not implemented in Lua (needs special recompilation of DUALNUM). The correct double precision M.EPSILON should be 2^-52 ~ 2.221e-16 so the user must be able to set its default precision (assuming IEEE 754 representation). Moreover, values like 1e-7 or 1e-12 are more about uncertainties than precision. And 1e-7 from you commit is too small for single precision (FLT_EPSILON = 2^-23 ~ 1.1921e-7 > 1e-7).

Why not simply use:

single precision (if any): M.EPSILON = 2^4 * 2^-23 ~ 2e-6
double precision: M.EPSILON = 2^8 * 2^-52 ~ 6e-14

Larger epsilon may be not be strict enough to detect bugs, even in simple cases… If precision error reach this level of EPSILON, something wrong is going on and/or the test itself introduces enough uncertainty that the user must provide appropriate epsilon. For example:

print((60-math.deg(math.pi/3))/2^-52) —> 32

Tells you that despite of its simplicity, radian to degree conversion is not accurate for 60 degrees (i.e. pi/3) and needs a margin of 32 * 2^-52 = 32 * DBL_EPSILON… Hiding this fact is not good for users, it is often better to give an opportunity to the users to understand what’s happening (and maybe learn something useful).

Just to not confuse people with this example, what I wanted to say here is that assertAlmostEquals is using margin as an absolute error. The case above was an example to show that sometime (most of the time…) absolute error is not suitable and the user must 1) be aware (with not to lazy EPSILON), 2) use the appropriate error check, typically the relative error:

( 60-math.deg( math.pi/3))/2^-52 = 32 EPS
(120-math.deg(2_math.pi/3))/2^-52 = 64 EPS
(240-math.deg(4_math.pi/3))/2^-52 = 128 EPS
etc…

the absolute error is proportional to angle (as expected), but the relative error is stable:

( 60/math.deg( math.pi/3)-1)/2^-52 = 1 EPS
(120/math.deg(2_math.pi/3)-1)/2^-52 = 1 EPS
(240/math.deg(4_math.pi/3)-1)/2^-52 = 1 EPS

That is why personally, I prefer to explain than to hide the problem and keep the tests as strict as possible. There is no best EPSILON for absolute error, so I would even go for
M.EPSILON = 2^-52 * 2 and put some wording in the documentation.

Best,
Laurent.

[ldeniau]

On 27 Jul 2016, at 08:44, Bluebird75 notifications@github.com wrote:

For merging the changes, I have my internal policy of merging functionality along with unit-test that validate it. This way, I can ensure that core luaunit is always as stable as possible.

So, either Laurent, you add more unit-tests for the functionality which you have added, or you leave it as it is and I will write the unit-test myself.

I am a bit short in time (I have myself thousands of tests to write), and it is often better that tests are written by third party then validated by authors of the code. I propose that you start writing tests following your policy, and contact me when you want be to review them. I may then add more tests following your style for corner cases if you agree.
I already appreciate the effort you have been putting into contributing to luaunit so I am hesitant to ask for more...

I can contribute moderately during the nights and the week-end, so do not hesitate.

Best,
Laurent.

[n1tehawk]

I'm currently thinking about a (helper) function that could be used to specify the desired error "margin" (boundary), both as absolute value and 'relative' to the internal precision (in terms of ε). This function would handle the two optional parameters limit and epsilons from something like assertAlmostEquals(actual, expected, limit, epsilons) and assertNotAlmostEquals(actual, expected, limit, epsilons). The epsilons parameter would provide a convenient way of saying "no more than 5*ε".

--[[ function to determine an error margin

`limit` represents an "absolute" boundary that should not be exceeded. For
user's convenience, it's possible to pass a non-numeric argument (boolean
`true`) to select M.EPSILON as the default margin.

`epsilons` is a multiplicative factor that specifies the value in terms of
"multiples of M.EPSILON". (This may be useful to express error margins relative
to the numeric precision that the underlying Lua implementation offers.)

If two parameters are specified, both will be interpreted as described above,
and then this function will select the smaller (absolute) value of the two,
i.e. the narrower margin. This reflects whatever condition of margin(limit, nil)
and margin(nil, epsilons) would be "more strict".

margin()            0
margin(2E-7)        2E-7
margin(2E-7, 3)     math.min(2E-7, 3 * M.EPSILON)
margin(true)        M.EPSILON
margin(nil, 1)      M.EPSILON
margin(true, 0.5)   math.min(M.EPSILON, 0.5 * M.EPSILON)  = M.EPSILON / 2
]]
local function margin(limit, epsilons)

    local function relaxedMin(a, b)
        -- Return the minimum of (a, b), i.e. the smaller value between the two.
        -- Either value may be `nil`, in that case the other is returned - if
        -- both a and b are nil, the function will return nil too.
        if a ~= nil then
            if b == nil then
                return a
            end
            if a < b then
                return a
            end
        end
        return b
    end

    if limit then
        limit = tonumber(limit) or M.EPSILON
    end
    if epsilons then
        epsilons = tonumber(epsilons)
                   or error("Malformed 'epsilons' parameter: " .. epsilons)
        epsilons = epsilons * M.EPSILON
    end
    -- default value below open to debate: 0 or M.EPSILON ?
    return relaxedMin(limit, epsilons) or 0
end

Undoubtedly the function is to respect any user-provided zero value (one of your primary concerns), i.e. margin(0, nil), margin(0, 1E99), margin(1E99, 0) etc. will always result in 0. What I'm not entirely sure about is how to handle margin() with no parameters (= margin(nil, nil)). Following the logic you currently suggested, this should default to 1*ε (M.EPSILON). When initially writing the above function, I went for 0 - requiring users to explicitly request the default with something like margin(true) or margin(nil, 1). What's your take on this?

Regards, NiteHawk

[bluebird75]

Regarding marginBoost, @ldeniau , you have convinced me that it should go away.

For this assertAlmostEquals, we will face two kind of users :

1. users familiar with the topic of scientific computing, like Laurent. They want absolute precision, know what they are doing and marginBoost is a distraction for them.
2. users non familiar with scientific computing, like myself. If they are faced with calculation errors, either they will have to ignore it with a big margin, or they will have to understand how to define their margin and understand better the different rules behind it.

LuaUnit itself can not pretend to solve all the problems related to computing errors.

[bluebird75]

What we can do however is to provide explicit documentation support and link to further explanations.

[n1tehawk]

Philippe, regarding your remark in #77:

I wanted to clarify on EPSILON in the first place (what it means and why it is there), and deliberately did not change the "margin boost" logic in almostEquals().

At that point in time the state of this pull request was unclear to me, and I didn't want to break the way LuaUnit is currently working. I'm fine with any further improvents of that function that we eventually come up with here.

[EDIT] One way to achieve the desired "strictness" would be to change almostEquals() from an implicit margin/boost (which was Laurent's main criticism as I understood it) to requiring an explicit one (if desired).

Regards, NiteHawk

[ldeniau]

Hi,

AFAICS, I don’t have remark in #77. Could you be more precise?

Concerning EPSILON, and Margin boost, we already had lengthy discussion and my point of view was already clear enough. EPSILON as a default value when the user does not provide one is fine. Margin boost is an heresy and must absolutely be removed, because it hides bugs! Moreover, you will never be able to set Margin boost (or even Epsilon) for general use, so in all cases, it brings more problem than it solves.

On the other hand, making assertAlmostEqual an equivalent to assertEqual with a user margin (or EPSILON if not provided) would be much more useful (and sharing the same underlying code). In many many tests, with have to loop over tables with assertAlmostEqual where assertEqual would nicely do the job (but without margin).

Best,
Laurent.

On 04 Nov 2016, at 16:00, NiteHawk notifications@github.com wrote:

Laurent, regarding your remark in #77 #77:

I wanted to clarify on EPSILON in the first place (what it means and why it is there), and deliberately did not change the "margin boost" logic in almostEquals().

At that point in time the state of this pull request was unclear to me, and I didn't want to break the way LuaUnit is currently working. I'm fine with any further improvents of that function that we eventually come up with here.

Regards, NiteHawk

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub #68 (comment), or mute the thread https://github.com/notifications/unsubscribe-auth/ABKFz6-vn0znDpFDZKUv1czBBvMx7d02ks5q60h_gaJpZM4JVETw.

[n1tehawk]

AFAICS, I don’t have remark in #77. Could you be more precise?

I'm sorry, I addressed you by mistake instead of Philippe before editing my comment above.

[n1tehawk]

Let's summarize / cut down on the status of this PR:

Half of the (12) commits above have already been integrated into master

• f4f11e8 ("--exclude" option) was merged in e18d7ce
• f8efb3e ("--count" option) was merged in 9a4fda2
• 6b00239 (assertIsNaN and assertIsNotNaN) was merged in 3c50859
• 9c867a9 (strict assertTrue and assertFalse) was merged in 5087511
• b728380 (assertIsInf and assertIsNotInf) was merged in 00fb713
• the 5835be7 fixup was included (squashed) into 00fb713

The M.EPSILON-related changes (d643f82 and f5d3891) should have been addressed by 151bdfc.

This leaves us with (only) the core changes made to almostEquals() and friends. Basically the remainder of eaf335a, 248da00, 86d594b and d346273 can be squashed into a single commit. I've done this here, on top op current master.

Corresponding unit tests are here, and I'd also propose a change that outputs the actual numerical difference by which margin was exceeded (or missed) in the failure message, to avoid potential confusion of users: see here.

Regards, NiteHawk

[n1tehawk]

Can we finalize this pull request somehow?

@ldeniau I think the set of changes in master...n1tehawk:20161105_pull68 should contain anything that's left? If you're okay with that, I can create a corresponding PR; and (after it has been merged) this one could be closed...

Regards, NiteHawk

[bluebird75]

For me, all changes have been indeed integrated. I'll close it soon.

[n1tehawk]

The change that modifies almostEquals() to fully respect the user-provided margin (instead of silently adding M.EPSILON on top of it) is still missing.

[bluebird75]

Closing this PR. We will cherry-pick the count of assertions in another PR.