improve error reporting in some tests

[cnassau]

In case of failure the routines

• _test_associativity
• _test_distributivity
• _test_one
• _test_prod
  should report the elements that failed.

The attached patch just enhances the error messages and adds some test cases similar to the following:

sage: import types
sage: P.<x,y>=PolynomialRing(QQbar,"x,y")
sage: x._mul_ = types.MethodType(lambda a,b: 0,x)
sage: y._mul_ = types.MethodType(lambda a,b: a,x)
sage: P._test_associativity(elements=[x,y])
Traceback (most recent call last):
...
AssertionError: (x*y)*z != x*(y*z) for (x,y,z) = (y,y,x)

Component: categories

Author: Christian Nassau

Reviewer: Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/13975

[tscrim]

comment:2

You missed a test for rings/real_lazy.pyx (see patchbot log) and I think you should pick a convention and stick with it (the associativity test differs from the others).

Best,

Travis

[cnassau]

comment:3

Attachment: cnatestfail.patch.gz

Replying to @tscrim:

You missed a test for rings/real_lazy.pyx (see patchbot log) and I think you should pick a convention and stick with it (the associativity test differs from the others).

Best,

Travis

I have now changed the failure message in the associativity test case and also fixed the doctest in real_lazy.pyx.

Cheers,
Christian

[tscrim]

comment:4

Looks good to me now. Thank you.

Best,

Travis

[tscrim]

Reviewer: Travis Scrimshaw

[nthiery]

comment:5

I don't necessarily agree. The right way to access the failing elements is to use post mortem introspection with the debugger, as described in TestSuite. I agree it is a little improvement if the elements are printed right away. However, with the current patch, this does reduce the lisibility of the test function: I want the mathematical test to be written in a way that is as close as possible to the math.

Cheers,
Nicolas

[cnassau]

comment:6

Replying to @nthiery:

However, with the current patch, this does reduce the lisibility of the test function: I want the mathematical test to be written in a way that is as close as possible to the math.

I suggested this patch because it would have saved me a lot of time recently. Now you're saying we can't have more helpful error messages because

tester.assert_(self.prod([x]) == x)

is so much more readable than

tester.assertEqual(self.prod([x]),x,LazyFormat("self.prod([x]) != x for x=%s") % (x,))

Frankly, I find this un peu ridicule...

Cheers,

Christian

[nthiery]

comment:7

Replying to @cnassau:

I suggested this patch because it would have saved me a lot of time recently. Now you're saying we can't have more helpful error messages because

tester.assert_(self.prod([x]) == x)

is so much more readable than

tester.assertEqual(self.prod([x]),x,LazyFormat("self.prod([x]) != x for x=%s") % (x,))

Frankly, I find this un peu ridicule...

Sorry for being picky, but I handcrafted this piece of code with much
attention and a strong rationale (whether you agree or not with that
rationale is yours to decide). I have converged to this solution after
experimenting in practice with many other solutions for months.
Postmortem introspection with the debugger really is the way to
go. Having the element printed out isn't worth much in practice
because anyway the next thing you want to do is reproduce the
error. And 90% of the time recreating the elements from their string
output is just painful. Whereas with the debugger you can immediately
access the elements, run whatever calculation you need to , explore,
etc.

The rationale above assumes that someone encountering such errors is a
minimum of a dev. And we want devs to knows how to read a stack trace
and how to use the debugger.

That being said, I totally support efforts in the following
directions:

• Pointing people to the right tool
• Training people to the right tool
• Having the right tool work right (in particular having the debugger work in the notebook)

I guess I can live, for the most used tests, with an idiom like:

    tester.assert_(   (x * y) * z == x * (y * z) ,
                   LazyFormat("(x*y)*z != x*(y*z) for (x,y,z) = (%s,%s,%s)") % (x,y,z) )

to make the learning curve a bit smoother. Or even

    tester.assertEqual(   (x * y) * z,  x * (y * z) ,
                   LazyFormat("(x*y)*z != x*(y*z) for (x,y,z) = (%s,%s,%s)") % (x,y,z) )

But please make sure that the formula is well highlighted.
Legibility and conciseness is critical. For in the long run we want to
have such tests all over the Sage library. If there is not a concise,
legible, and uniform idiom, developers just won't write those tests.

If you want to see some non trivial examples where we simply can't
afford to put long Lazy format strings, see e.g.
sage.combinat.root_system.root_lattice_realizations.ParentMethods._test_root_lattice_realization.

Cheers,
Nicolas

[cnassau]

comment:8

Hi Nicolas,

Thank you very much for these explanations - you've clearly put a lot of work and thought into these issues so I won't argue with you. But do I understand you correctly, that

tester.assert_(   (x * y) * z == x * (y * z) ,
               LazyFormat("(x*y)*z != x*(y*z) for (x,y,z) = (%s,%s,%s)") % (x,y,z) )

would find your approval? Because I then just might re-format my patch in this style and set it to "need review" again... ;-)

Cheers,

Christian

[nthiery]

comment:9

Replying to @cnassau:

Thank you very much for these explanations - you've clearly put a lot of work and thought into these issues so I won't argue with you. But do I understand you correctly, that

tester.assert_(   (x * y) * z == x * (y * z) ,
               LazyFormat("(x*y)*z != x*(y*z) for (x,y,z) = (%s,%s,%s)") % (x,y,z) )

would find your approval? Because I then just might re-format my patch in this style and set it to "need review" again... ;-)

Oops, sorry, I let that slip away in my mailbox.

Yes, that would be alright indeed!