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:  sage6.5 
Component:  documentation  Keywords:  
Cc:  KarlDieter 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: 
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
Branch:  → u/ncohen/17549 

Status:  new → needs_review 
comment:2 Changed 8 years ago by
Commit:  → 2f9a44c1ccd5ef3e087dc0a54da42883e83db70a 

comment:3 Changed 8 years ago by
Commit:  2f9a44c1ccd5ef3e087dc0a54da42883e83db70a → 6ff61bdd86d0031d12e13cf89d0e4041da343bc3 

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

comment:4 followup: 5 Changed 8 years ago by
Status:  needs_review → needs_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
.
comment:5 followup: 6 Changed 8 years ago by
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 Changed 8 years ago by
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.93889390390723e18 6.24500451351651e17] [ 4.44089209850063e16 1.00000000000000 1.11022302462516e16] [1.94289029309402e16 1.38777878078145e17 1.00000000000000] sage: M^1 * M # tol 2e16 [ 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 nonzero entries are checked with **relative** tolerance.
comment:7 Changed 8 years ago by
Commit:  6ff61bdd86d0031d12e13cf89d0e4041da343bc3 → 229d4f446e9b1b71df2849af28273df67ef08daa 

Branch pushed to git repo; I updated commit sha1. New commits:
229d4f4  trac #17549: multiple numerical values in doctest output

comment:8 Changed 8 years ago by
Status:  needs_work → needs_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 followup: 10 Changed 8 years ago by
Status:  needs_review → needs_work 

This is wrong:
**Zero** values are always tested relative error.
comment:10 Changed 8 years ago by
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
Oh. Sorry. Absolute. And the sentence is not even correct. Will be fixed in a second.
Nathann
comment:12 Changed 8 years ago by
What was written:
this defaults to relative error except when the expected value is exactly zero::
comment:13 followup: 14 Changed 8 years ago by
There is a difference between "X is always true" and "X is the default".
comment:14 followup: 19 Changed 8 years ago by
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 followup: 18 Changed 8 years ago by
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
Commit:  229d4f446e9b1b71df2849af28273df67ef08daa → 9becf3d47fde28661ac15b356c7af188a5ef0de3 

Branch pushed to git repo; I updated commit sha1. New commits:
9becf3d  trac #17549: Comment on zero values

comment:17 Changed 8 years ago by
Status:  needs_work → needs_review 

comment:18 Changed 8 years ago by
Status:  needs_review → needs_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
 nonzero values: tested with relative tolerance by default
But abs
or rel
can override the default for both cases.
comment:19 followup: 21 Changed 8 years ago by
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
Commit:  9becf3d47fde28661ac15b356c7af188a5ef0de3 → 526c1662536e48b34c0e4adda2321484b56ad238 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
526c166  trac #17549: Comment on zero values

comment:21 Changed 8 years ago by
Status:  needs_work → needs_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
Commit:  526c1662536e48b34c0e4adda2321484b56ad238 → 8ab367fdfef0c8c56cdda4b6705ba55e4aebf5c6 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
8ab367f  trac #17549: Comment on zero values

comment:23 followup: 24 Changed 8 years ago by
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 Changed 8 years ago by
Yo !
sage t optional=internet
only runs the internet tests, and not any of the tests that are run by default. You almost always wantsage 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
Status:  needs_review → needs_work 

The numerical values returned by the line is
> last word should be are
comment:26 Changed 8 years ago by
I think the remark (rounding error/floating point arithmetic)
is important enough that it should be clarified and expanded further: what about (due to systemdependent floating point arithmetic or math libraries or nondeterministic algorithms)
.
comment:27 Changed 8 years ago by
the representation of complex numbers, matrices, or polynomials involve
> change last word to usually involves
comment:28 Changed 8 years ago by
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
Reviewers:  → Jeroen Demeyer 

comment:30 Changed 8 years ago by
Commit:  8ab367fdfef0c8c56cdda4b6705ba55e4aebf5c6 → 1cbe479fd2a69d436fce8880d40275e41aaeb36b 

Branch pushed to git repo; I updated commit sha1. New commits:
1cbe479  trac #17549: Review

comment:31 followup: 33 Changed 8 years ago by
Yo !
I implemented all changes that you asked. I only omitted "math libraries" from
(due to systemdependent floating point arithmetic or math libraries or nondeterministic algorithms)
Because in my understanding, most of what Sage contains is a math library. Fortunately what causes problems in the thirdparty math librarires that we use is precisely floatingpoint airethmetic or nondeterministic algorithms, so that's fine.
Nathann
comment:32 Changed 8 years ago by
Status:  needs_work → needs_review 

comment:33 followup: 35 Changed 8 years ago by
I actually meant
systemdependent (floating point arithmetic or math libraries) or nondeterministic algorithms
i.e. systemdependent math libraries, not Sage math libraries.
Fortunately what causes problems in the thirdparty math librarires that we use is precisely floatingpoint airethmetic or nondeterministic 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
Commit:  1cbe479fd2a69d436fce8880d40275e41aaeb36b → 45aadab79b2eceae52a6ade5575848304f1a4048 

Branch pushed to git repo; I updated commit sha1. New commits:
45aadab  trac #17549: Review

comment:37 followups: 40 41 Changed 8 years ago by
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 KarlDieter wants to give it a read since he's the only native English speaker...
comment:38 Changed 8 years ago by
A line flagged with ``optional  keyword``, it is not tested
: remove ", it
"
comment:39 Changed 8 years ago by
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 Changed 8 years ago by
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 Changed 8 years ago by
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 KarlDieter 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 "32bit" and "64bit" 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
Commit:  45aadab79b2eceae52a6ade5575848304f1a4048 → 0ebf12ac4f21f78ebe2078b910624c1e97e3512d 

Branch pushed to git repo; I updated commit sha1. New commits:
0ebf12a  trac #17549: Review

comment:43 followup: 44 Changed 8 years ago by
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#runoptionaltests
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 "32bit" and "64bit" 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 followup: 45 Changed 8 years ago by
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 Changed 8 years ago by
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
Branch:  u/ncohen/17549 → u/jdemeyer/ticket/17549 

Created:  Dec 26, 2014, 4:31:13 AM → Dec 26, 2014, 4:31:13 AM 
Modified:  Dec 31, 2014, 9:28:35 AM → Dec 31, 2014, 9:28:35 AM 
comment:47 Changed 8 years ago by
Commit:  0ebf12ac4f21f78ebe2078b910624c1e97e3512d → e6da3d75f350ae019ba27070ff20e4df3943033a 

Status:  needs_review → positive_review 
New commits:
e6da3d7  Typo

comment:49 Changed 8 years ago by
I fixed one more typo which was not your fault :)
Oh, right.... O_o
Thanks for the review !
Nathann
comment:50 followup: 51 Changed 8 years ago by
I won't object to the positive review, but I still think that we should explain what relative vs. absolute tolerance is.
comment:51 Changed 8 years ago by
Branch:  u/jdemeyer/ticket/17549 → public/17549 

Commit:  e6da3d75f350ae019ba27070ff20e4df3943033a → 19d6ff27e6e2542143dbe546968bdca198323d4c 
Status:  positive_review → needs_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:
19d6ff2  trac #17549: Link toward an explanation of absolute/relative

comment:53 Changed 8 years ago by
Branch:  public/17549 → 19d6ff27e6e2542143dbe546968bdca198323d4c 

Resolution:  → fixed 
Status:  positive_review → closed 
Branch pushed to git repo; I updated commit sha1. New commits:
trac #17549: Rephrase the 'doctest flags' section of the developer's manual