Opened 8 years ago

Closed 8 years ago

#17549 closed enhancement (fixed)

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

Reported by: Nathann Cohen Owned by:
Priority: major Milestone: sage-6.5
Component: documentation Keywords:
Cc: Karl-Dieter Crisman, Vincent Delecroix, Thierry Monteil Merged in:
Authors: Nathann Cohen Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: 19d6ff2 (Commits, GitHub, GitLab) Commit: 19d6ff27e6e2542143dbe546968bdca198323d4c
Dependencies: Stopgaps:

Status badges

Description

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

Nathann

Change History (53)

comment:1 Changed 8 years ago by Nathann Cohen

Branch: u/ncohen/17549
Status: newneeds_review

comment:2 Changed 8 years ago by git

Commit: 2f9a44c1ccd5ef3e087dc0a54da42883e83db70a

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

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

comment:3 Changed 8 years ago by git

Commit: 2f9a44c1ccd5ef3e087dc0a54da42883e83db70a6ff61bdd86d0031d12e13cf89d0e4041da343bc3

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

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

comment:4 Changed 8 years ago by Jeroen Demeyer

Status: needs_reviewneeds_work

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.

Last edited 8 years ago by Jeroen Demeyer (previous) (diff)

comment:5 in reply to:  4 ; Changed 8 years ago by Nathann Cohen

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

comment:6 in reply to:  5 Changed 8 years ago by Jeroen Demeyer

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.

comment:7 Changed 8 years ago by git

Commit: 6ff61bdd86d0031d12e13cf89d0e4041da343bc3229d4f446e9b1b71df2849af28273df67ef08daa

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

229d4f4trac #17549: multiple numerical values in doctest output

comment:8 Changed 8 years ago by Nathann Cohen

Status: needs_workneeds_review

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

comment:9 Changed 8 years ago by Jeroen Demeyer

Status: needs_reviewneeds_work

This is wrong:

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

comment:10 in reply to:  9 Changed 8 years ago by Nathann Cohen

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

comment:11 Changed 8 years ago by Nathann Cohen

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

Nathann

comment:12 Changed 8 years ago by Jeroen Demeyer

What was written:

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

comment:13 Changed 8 years ago by Jeroen Demeyer

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

comment:14 in reply to:  13 ; Changed 8 years ago by Nathann Cohen

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

comment:15 Changed 8 years ago by Nathann Cohen

So is the rule like that ?

  • Single output: absolute is the default for 0 output
  • Multiple output: zeroes are always tested with absolute ?

Nathann

comment:16 Changed 8 years ago by git

Commit: 229d4f446e9b1b71df2849af28273df67ef08daa9becf3d47fde28661ac15b356c7af188a5ef0de3

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

9becf3dtrac #17549: Comment on zero values

comment:17 Changed 8 years ago by Nathann Cohen

Status: needs_workneeds_review

comment:18 in reply to:  15 Changed 8 years ago by Jeroen Demeyer

Status: needs_reviewneeds_work

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.

Last edited 8 years ago by Jeroen Demeyer (previous) (diff)

comment:19 in reply to:  14 ; Changed 8 years ago by Jeroen Demeyer

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.

comment:20 Changed 8 years ago by git

Commit: 9becf3d47fde28661ac15b356c7af188a5ef0de3526c1662536e48b34c0e4adda2321484b56ad238

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

526c166trac #17549: Comment on zero values

comment:21 in reply to:  19 Changed 8 years ago by Nathann Cohen

Status: needs_workneeds_review

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

What about this last (updated) commit ?

Nathann

comment:22 Changed 8 years ago by git

Commit: 526c1662536e48b34c0e4adda2321484b56ad2388ab367fdfef0c8c56cdda4b6705ba55e4aebf5c6

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

8ab367ftrac #17549: Comment on zero values

comment:23 Changed 8 years ago by Volker Braun

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.

comment:24 in reply to:  23 Changed 8 years ago by Nathann Cohen

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

comment:25 Changed 8 years ago by Jeroen Demeyer

Status: needs_reviewneeds_work

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

comment:26 Changed 8 years ago by Jeroen Demeyer

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).

comment:27 Changed 8 years ago by Jeroen Demeyer

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

comment:28 Changed 8 years ago by Jeroen Demeyer

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.

comment:29 Changed 8 years ago by Jeroen Demeyer

Reviewers: Jeroen Demeyer

comment:30 Changed 8 years ago by git

Commit: 8ab367fdfef0c8c56cdda4b6705ba55e4aebf5c61cbe479fd2a69d436fce8880d40275e41aaeb36b

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

1cbe479trac #17549: Review

comment:31 Changed 8 years ago by Nathann Cohen

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

comment:32 Changed 8 years ago by Nathann Cohen

Status: needs_workneeds_review

comment:33 in reply to:  31 ; Changed 8 years ago by Jeroen Demeyer

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.

comment:34 Changed 8 years ago by git

Commit: 1cbe479fd2a69d436fce8880d40275e41aaeb36b45aadab79b2eceae52a6ade5575848304f1a4048

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

45aadabtrac #17549: Review

comment:35 in reply to:  33 Changed 8 years ago by Nathann Cohen

That's what I meant.

Done.

Nathann

comment:36 Changed 8 years ago by Nathann Cohen

Is there anything else Jeroen ?

comment:37 Changed 8 years ago by Volker Braun

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...

comment:38 Changed 8 years ago by Jeroen Demeyer

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

comment:39 Changed 8 years ago by Jeroen Demeyer

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.

comment:40 in reply to:  37 Changed 8 years ago by Jeroen Demeyer

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.

comment:41 in reply to:  37 Changed 8 years ago by John Palmieri

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.)

comment:42 Changed 8 years ago by git

Commit: 45aadab79b2eceae52a6ade5575848304f1a40480ebf12ac4f21f78ebe2078b910624c1e97e3512d

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

0ebf12atrac #17549: Review

comment:43 Changed 8 years ago by Nathann Cohen

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

comment:44 in reply to:  43 ; Changed 8 years ago by Jeroen Demeyer

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.

comment:45 in reply to:  44 Changed 8 years ago by Nathann Cohen

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

comment:46 Changed 8 years ago by Jeroen Demeyer

Branch: u/ncohen/17549u/jdemeyer/ticket/17549
Created: Dec 26, 2014, 4:31:13 AMDec 26, 2014, 4:31:13 AM
Modified: Dec 31, 2014, 9:28:35 AMDec 31, 2014, 9:28:35 AM

comment:47 Changed 8 years ago by Jeroen Demeyer

Commit: 0ebf12ac4f21f78ebe2078b910624c1e97e3512de6da3d75f350ae019ba27070ff20e4df3943033a
Status: needs_reviewpositive_review

New commits:

e6da3d7Typo

comment:48 Changed 8 years ago by Jeroen Demeyer

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

comment:49 in reply to:  48 Changed 8 years ago by Nathann Cohen

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

Oh, right.... O_o

Thanks for the review !

Nathann

comment:50 Changed 8 years ago by John Palmieri

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

comment:51 in reply to:  50 Changed 8 years ago by Nathann Cohen

Branch: u/jdemeyer/ticket/17549public/17549
Commit: e6da3d75f350ae019ba27070ff20e4df3943033a19d6ff27e6e2542143dbe546968bdca198323d4c
Status: positive_reviewneeds_review

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:

19d6ff2trac #17549: Link toward an explanation of absolute/relative

comment:52 Changed 8 years ago by John Palmieri

Status: needs_reviewpositive_review

Great, thanks!

comment:53 Changed 8 years ago by Volker Braun

Branch: public/1754919d6ff27e6e2542143dbe546968bdca198323d4c
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.