Rephrase the 'doctest flags' section of the developer's manual

[ncohen]

As with previous patches, this branch is meant to make this section easier to read/browse through.

Nathann

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​2f9a44c

trac #17549: Rephrase the 'doctest flags' section of the developer's manual

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​6ff61bd

trac #17549: The "doctesting Sage" section explains how to run the doctests, not how to write them

[jdemeyer]

The tolerance checks do not "work with complex numbers". The example only works because a complex number is seen as <real number><real number>*I.

Also, replace **not tested:** by **not tested**

[ncohen]

The tolerance checks do not "work with complex numbers". The example only works because a complex number is seen as <real number><real number>*I.

So does it work with complex number in general, or not ? Could you tell me how you would like me to rephrase that ? Or could you add a commit ? I do not see exactly how I should say it.

Nathann

[jdemeyer]

Perhaps add something along the lines of

If a doctest with tolerance contains several numbers, each of them is checked individually::

    sage: print "The sum of 1 and 1 equals 5"  # abs tol 1
    The sum of 2 and 2 equals 4

As a consequence of this, tests involving complex numbers, matrices or polynomials usually work as expected::

    sage: <complex number example here>
    sage: M = Matrix(RR, 3, 3, [-13, 1, 1, -3, 0, -2, -4, -2, 0])
    sage: M^-1 * M   # actual result
    [     1.00000000000000  6.93889390390723e-18 -6.24500451351651e-17]
    [ 4.44089209850063e-16      1.00000000000000  1.11022302462516e-16]
    [-1.94289029309402e-16 -1.38777878078145e-17      1.00000000000000]
    sage: M^-1 * M   # tol 2e-16
    [ 1.00000000000000 0.000000000000000 0.000000000000000]
    [0.000000000000000  1.00000000000000 0.000000000000000]
    [0.000000000000000 0.000000000000000  1.00000000000000]

Note that, in the last example, the zeros are checked with **absolute** tolerance, while the non-zero entries are checked with **relative** tolerance.

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​229d4f4

trac #17549: multiple numerical values in doctest output

[ncohen]

What do you think of that ? I rather like the result, I find it clearer and more complete. I simplified your example a bit, if you do not mind.

Nathann

[jdemeyer]

This is wrong:

**Zero** values are always tested relative error.

[ncohen]

This is wrong:

**Zero** values are always tested relative error.

Why is it wrong ? I thought that it is what was written in the manual already.

Nathann

[ncohen]

Oh. Sorry. Absolute. And the sentence is not even correct. Will be fixed in a second.

Nathann

[jdemeyer]

What was written:

this defaults to relative error except
when the expected value is exactly zero::

[jdemeyer]

There is a difference between "X is always true" and "X is the default".

[ncohen]

There is a difference between "X is always true" and "X is the default".

Yes, but wasn't it written somewhere that when you had multiple output and one of them was a zero then the absolute was also used ?

Nathann

[ncohen]

So is the rule like that ?

• Single output: absolute is the default for 0 output

• Multiple output: zeroes are always tested with absolute ?

Nathann

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​9becf3d

trac #17549: Comment on zero values

[jdemeyer]

Replying to ncohen:

So is the rule like that ?

• Single output: absolute is the default for 0 output

• Multiple output: zeroes are always tested with absolute ?

No!

The rule is:

• zero values: tested with absolute tolerance by default

• non-zero values: tested with relative tolerance by default

But abs or rel can override the default for both cases.

[jdemeyer]

Replying to ncohen:

Yes, but wasn't it written somewhere that when you had multiple output

Every value is tested independently, every value has its own default.

[git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

​526c166

trac #17549: Comment on zero values

[ncohen]

Every value is tested independently, every value has its own default.

What about this last (updated) commit ?

Nathann

[git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

​8ab367f

trac #17549: Comment on zero values

[vbraun]

sage -t --optional=internet only runs the internet tests, and not any of the tests that are run by default. You almost always want sage -t --optional=sage,internet, and similar for other optional packages. IMHO this should be spelled out in more detail as it is a frequent cause for confusion.

[ncohen]

Yo !

sage -t --optional=internet only runs the internet tests, and not any of the tests that are run by default. You almost always want sage -t --optional=sage,internet, and similar for other optional packages.

True. Actually, I never ran --optional without having 'sage' inside. Should we change that instead ? I can't think of any good reason for this kind of behaviour, as it does create confusion.

Nathann

[jdemeyer]

The numerical values returned by the line is -> last word should be are

[jdemeyer]

I think the remark (rounding error/floating point arithmetic) is important enough that it should be clarified and expanded further: what about (due to system-dependent floating point arithmetic or math libraries or non-deterministic algorithms).

[jdemeyer]

the representation of complex numbers, matrices, or polynomials involve -> change last word to usually involves

[jdemeyer]

The sentence Neither of this applies to files or directories which are explicitly given as command line arguments: those are always tested. should be outside the list, since it refers both to files and directories. It should not be just in the "directory" item.

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​1cbe479

trac #17549: Review

[ncohen]

Yo !

I implemented all changes that you asked. I only omitted "math libraries" from (due to system-dependent floating point arithmetic or math libraries or non-deterministic algorithms)

Because in my understanding, most of what Sage contains is a math library. Fortunately what causes problems in the third-party math librarires that we use is precisely floating-point airethmetic or non-deterministic algorithms, so that's fine.

Nathann

[jdemeyer]

I actually meant

system-dependent (floating point arithmetic or math libraries) or non-deterministic algorithms

i.e. system-dependent math libraries, not Sage math libraries.

Fortunately what causes problems in the third-party math librarires that we use is precisely floating-point airethmetic or non-deterministic algorithms, so that's fine.

That's not actually true. You could have exactly the same hardware with different deterministic results for log() because the system uses a different library to compute log(). That's what I meant.

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​45aadab

trac #17549: Review

[ncohen]

That's what I meant.

Done.

Nathann

[ncohen]

Is there anything else Jeroen ?

[vbraun]

What about the --optional=sage,keyword? The rationale was that you might be able to speed up testing by only running the optional tests, but in practice that pretty much never works (since some intermediate steps tend to not have the #optional tag) and I've never used it. IMHO: Either document it clearly or change the behavior.

Maybe Karl-Dieter wants to give it a read since he's the only native English speaker...

[jdemeyer]

A line flagged with ``optional - keyword``, it is not tested: remove ", it"

[jdemeyer]

In the sentence the representation of complex numbers, matrices, or polynomials usually involve ..., I really think that it should be involves since "representation" is singular.

[jdemeyer]

Replying to vbraun:

What about the --optional=sage,keyword? The rationale was that you might be able to speed up testing by only running the optional tests, but in practice that pretty much never works (since some intermediate steps tend to not have the #optional tag) and I've never used it. IMHO: Either document it clearly or change the behavior.

I wouldn't change the behavior now, but I agree that it should be documented.

[jhpalmieri]

Replying to vbraun:

What about the --optional=sage,keyword? The rationale was that you might be able to speed up testing by only running the optional tests, but in practice that pretty much never works (since some intermediate steps tend to not have the #optional tag) and I've never used it. IMHO: Either document it clearly or change the behavior.

I agree that this should be documented.

Maybe Karl-Dieter wants to give it a read since he's the only native English speaker...

The English looks okay to me.

Two more suggestions: we should define relative and absolute tolerance, or at least give a link to a definition. Also, maybe we should emphasize that the "32-bit" and "64-bit" flags go on the output lines, not the input lines the way the other flags do. (Either before or after the example: "Note that unlike the other flags, these keywords go on the *output* lines, not the input lines.)

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​0ebf12a

trac #17549: Review

[ncohen]

Hello guys !

What about the --optional=sage,keyword?

It is already documented in the chapter (that this patch renames to) "Running the doctests". The section about the 'optional' flag begins with a link toward that page

http://www.sagemath.org/doc/developer/doctesting.html#run-optional-tests

I think that it is sufficient for the moment, especially since we all seem to agree that this behaviour should be changed so that 'sage' is always included in --optional=<a list>.

A line flagged with ``optional - keyword``, it is not tested: remove ", it"

Done.

I really think that it should be involves since "representation" is singular.

Done.

Also, maybe we should emphasize that the "32-bit" and "64-bit" flags go on the output lines.

Done

From the look of your comments I have no idea if you consider that this ticket can be switched to positive_review yet, but I would really appreciate it if you could make all your comments at once, next time. I have already pushed 6 different review commits here, and that is much more work than implementing at once all of the reviewer's comments.

Thanks,

Nathann

[jdemeyer]

Replying to ncohen:

I would really appreciate it if you could make all your comments at once, next time. I have already pushed 6 different review commits here, and that is much more work than implementing at once all of the reviewer's comments.

Don't forget that there was a lot of confusion about the tolerance stuff... I think that's mainly what caused the many commits. Also, it's not also always to give all reviewer comments at once: when there are major mistakes, I find it harder to concentrate on spelling errors. Also, sometimes I have comments about the review commits...

I was also busy doing other things these days, so I didn't have the time to make those review commits myself like I would usually do.

[ncohen]

Don't forget that there was a lot of confusion about the tolerance stuff...

Yeah that's true.

Well. Hopefully it is all good now :-P

Nathann

[jdemeyer]

New commits:

​e6da3d7

Typo

[jdemeyer]

I fixed one more typo which was not your fault :-)

[ncohen]

I fixed one more typo which was not your fault :-)

Oh, right.... O_o

Thanks for the review !

Nathann

[jhpalmieri]

I won't object to the positive review, but I still think that we should explain what relative vs. absolute tolerance is.

[ncohen]

I won't object to the positive review, but I still think that we should explain what relative vs. absolute tolerance is.

Can you review this new commit ?

Nathann

---

New commits:

​19d6ff2

trac #17549: Link toward an explanation of absolute/relative

[jhpalmieri]

Great, thanks!