Opened 6 years ago
Closed 6 years ago
#17549 closed enhancement (fixed)
Rephrase the 'doctest flags' section of the developer's manual
Reported by: | ncohen | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-6.5 |
Component: | documentation | Keywords: | |
Cc: | kcrisman, vdelecroix, tmonteil | Merged in: | |
Authors: | Nathann Cohen | Reviewers: | Jeroen Demeyer |
Report Upstream: | N/A | Work issues: | |
Branch: | 19d6ff2 (Commits) | 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 6 years ago by
- Branch set to u/ncohen/17549
- Status changed from new to needs_review
comment:2 Changed 6 years ago by
- Commit set to 2f9a44c1ccd5ef3e087dc0a54da42883e83db70a
comment:3 Changed 6 years ago by
- Commit changed from 2f9a44c1ccd5ef3e087dc0a54da42883e83db70a to 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 follow-up: ↓ 5 Changed 6 years ago by
- Status changed from needs_review to 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
.
Also, replace **not tested:**
by **not tested**
comment:5 in reply to: ↑ 4 ; follow-up: ↓ 6 Changed 6 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 in reply to: ↑ 5 Changed 6 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.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 6 years ago by
- Commit changed from 6ff61bdd86d0031d12e13cf89d0e4041da343bc3 to 229d4f446e9b1b71df2849af28273df67ef08daa
Branch pushed to git repo; I updated commit sha1. New commits:
229d4f4 | trac #17549: multiple numerical values in doctest output
|
comment:8 Changed 6 years ago by
- Status changed from needs_work to 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 follow-up: ↓ 10 Changed 6 years ago by
- Status changed from needs_review to needs_work
This is wrong:
**Zero** values are always tested relative error.
comment:10 in reply to: ↑ 9 Changed 6 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 6 years ago by
Oh. Sorry. Absolute. And the sentence is not even correct. Will be fixed in a second.
Nathann
comment:12 Changed 6 years ago by
What was written:
this defaults to relative error except when the expected value is exactly zero::
comment:13 follow-up: ↓ 14 Changed 6 years ago by
There is a difference between "X is always true" and "X is the default".
comment:14 in reply to: ↑ 13 ; follow-up: ↓ 19 Changed 6 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 follow-up: ↓ 18 Changed 6 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 6 years ago by
- Commit changed from 229d4f446e9b1b71df2849af28273df67ef08daa to 9becf3d47fde28661ac15b356c7af188a5ef0de3
Branch pushed to git repo; I updated commit sha1. New commits:
9becf3d | trac #17549: Comment on zero values
|
comment:17 Changed 6 years ago by
- Status changed from needs_work to needs_review
comment:18 in reply to: ↑ 15 Changed 6 years ago by
- Status changed from needs_review to 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
- non-zero values: tested with relative tolerance by default
But abs
or rel
can override the default for both cases.
comment:19 in reply to: ↑ 14 ; follow-up: ↓ 21 Changed 6 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 6 years ago by
- Commit changed from 9becf3d47fde28661ac15b356c7af188a5ef0de3 to 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 in reply to: ↑ 19 Changed 6 years ago by
- Status changed from needs_work to needs_review
Every value is tested independently, every value has its own default.
What about this last (updated) commit ?
Nathann
comment:22 Changed 6 years ago by
- Commit changed from 526c1662536e48b34c0e4adda2321484b56ad238 to 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 follow-up: ↓ 24 Changed 6 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 in reply to: ↑ 23 Changed 6 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 6 years ago by
- Status changed from needs_review to needs_work
The numerical values returned by the line is
-> last word should be are
comment:26 Changed 6 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 system-dependent floating point arithmetic or math libraries or non-deterministic algorithms)
.
comment:27 Changed 6 years ago by
the representation of complex numbers, matrices, or polynomials involve
-> change last word to usually involves
comment:28 Changed 6 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 6 years ago by
- Reviewers set to Jeroen Demeyer
comment:30 Changed 6 years ago by
- Commit changed from 8ab367fdfef0c8c56cdda4b6705ba55e4aebf5c6 to 1cbe479fd2a69d436fce8880d40275e41aaeb36b
Branch pushed to git repo; I updated commit sha1. New commits:
1cbe479 | trac #17549: Review
|
comment:31 follow-up: ↓ 33 Changed 6 years ago by
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 6 years ago by
- Status changed from needs_work to needs_review
comment:33 in reply to: ↑ 31 ; follow-up: ↓ 35 Changed 6 years ago by
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 6 years ago by
- Commit changed from 1cbe479fd2a69d436fce8880d40275e41aaeb36b to 45aadab79b2eceae52a6ade5575848304f1a4048
Branch pushed to git repo; I updated commit sha1. New commits:
45aadab | trac #17549: Review
|
comment:35 in reply to: ↑ 33 Changed 6 years ago by
That's what I meant.
Done.
Nathann
comment:36 Changed 6 years ago by
Is there anything else Jeroen ?
comment:37 follow-ups: ↓ 40 ↓ 41 Changed 6 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 Karl-Dieter wants to give it a read since he's the only native English speaker...
comment:38 Changed 6 years ago by
A line flagged with ``optional - keyword``, it is not tested
: remove ", it
"
comment:39 Changed 6 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 in reply to: ↑ 37 Changed 6 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 in reply to: ↑ 37 Changed 6 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 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 6 years ago by
- Commit changed from 45aadab79b2eceae52a6ade5575848304f1a4048 to 0ebf12ac4f21f78ebe2078b910624c1e97e3512d
Branch pushed to git repo; I updated commit sha1. New commits:
0ebf12a | trac #17549: Review
|
comment:43 follow-up: ↓ 44 Changed 6 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#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 ; follow-up: ↓ 45 Changed 6 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 in reply to: ↑ 44 Changed 6 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 6 years ago by
- Branch changed from u/ncohen/17549 to u/jdemeyer/ticket/17549
- Created changed from 12/26/14 04:31:13 to 12/26/14 04:31:13
- Modified changed from 12/31/14 09:28:35 to 12/31/14 09:28:35
comment:47 Changed 6 years ago by
- Commit changed from 0ebf12ac4f21f78ebe2078b910624c1e97e3512d to e6da3d75f350ae019ba27070ff20e4df3943033a
- Status changed from needs_review to positive_review
New commits:
e6da3d7 | Typo
|
comment:48 follow-up: ↓ 49 Changed 6 years ago by
I fixed one more typo which was not your fault :-)
comment:49 in reply to: ↑ 48 Changed 6 years ago by
I fixed one more typo which was not your fault :-)
Oh, right.... O_o
Thanks for the review !
Nathann
comment:50 follow-up: ↓ 51 Changed 6 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 in reply to: ↑ 50 Changed 6 years ago by
- Branch changed from u/jdemeyer/ticket/17549 to public/17549
- Commit changed from e6da3d75f350ae019ba27070ff20e4df3943033a to 19d6ff27e6e2542143dbe546968bdca198323d4c
- Status changed from positive_review to 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:52 Changed 6 years ago by
- Status changed from needs_review to positive_review
Great, thanks!
comment:53 Changed 6 years ago by
- Branch changed from public/17549 to 19d6ff27e6e2542143dbe546968bdca198323d4c
- Resolution set to fixed
- Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. New commits:
trac #17549: Rephrase the 'doctest flags' section of the developer's manual