Opened 5 years ago

Closed 5 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 5 years ago by ncohen

  • Branch set to u/ncohen/17549
  • Status changed from new to needs_review

comment:2 Changed 5 years ago by git

  • Commit set to 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 5 years ago by git

  • Commit changed from 2f9a44c1ccd5ef3e087dc0a54da42883e83db70a to 6ff61bdd86d0031d12e13cf89d0e4041da343bc3

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 follow-up: Changed 5 years ago by jdemeyer

  • 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**

Version 0, edited 5 years ago by jdemeyer (next)

comment:5 in reply to: ↑ 4 ; follow-up: Changed 5 years ago by 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

comment:6 in reply to: ↑ 5 Changed 5 years ago by 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.

comment:7 Changed 5 years ago by git

  • Commit changed from 6ff61bdd86d0031d12e13cf89d0e4041da343bc3 to 229d4f446e9b1b71df2849af28273df67ef08daa

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

229d4f4trac #17549: multiple numerical values in doctest output

comment:8 Changed 5 years ago by ncohen

  • 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: Changed 5 years ago by jdemeyer

  • 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 5 years ago by 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

comment:11 Changed 5 years ago by ncohen

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

Nathann

comment:12 Changed 5 years ago by jdemeyer

What was written:

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

comment:13 follow-up: Changed 5 years ago by jdemeyer

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

comment:14 in reply to: ↑ 13 ; follow-up: Changed 5 years ago by 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

comment:15 follow-up: Changed 5 years ago by 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

comment:16 Changed 5 years ago by git

  • Commit changed from 229d4f446e9b1b71df2849af28273df67ef08daa to 9becf3d47fde28661ac15b356c7af188a5ef0de3

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

9becf3dtrac #17549: Comment on zero values

comment:17 Changed 5 years ago by ncohen

  • Status changed from needs_work to needs_review

comment:18 in reply to: ↑ 15 Changed 5 years ago by jdemeyer

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

Last edited 5 years ago by jdemeyer (previous) (diff)

comment:19 in reply to: ↑ 14 ; follow-up: Changed 5 years ago by 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.

comment:20 Changed 5 years ago by git

  • Commit changed from 9becf3d47fde28661ac15b356c7af188a5ef0de3 to 526c1662536e48b34c0e4adda2321484b56ad238

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 5 years ago by ncohen

  • 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 5 years ago by git

  • Commit changed from 526c1662536e48b34c0e4adda2321484b56ad238 to 8ab367fdfef0c8c56cdda4b6705ba55e4aebf5c6

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

8ab367ftrac #17549: Comment on zero values

comment:23 follow-up: Changed 5 years ago by 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.

comment:24 in reply to: ↑ 23 Changed 5 years ago by 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

comment:25 Changed 5 years ago by jdemeyer

  • Status changed from needs_review to needs_work

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

comment:26 Changed 5 years ago by 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).

comment:27 Changed 5 years ago by jdemeyer

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

comment:28 Changed 5 years ago by 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.

comment:29 Changed 5 years ago by jdemeyer

  • Reviewers set to Jeroen Demeyer

comment:30 Changed 5 years ago by git

  • Commit changed from 8ab367fdfef0c8c56cdda4b6705ba55e4aebf5c6 to 1cbe479fd2a69d436fce8880d40275e41aaeb36b

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

1cbe479trac #17549: Review

comment:31 follow-up: Changed 5 years ago by 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

comment:32 Changed 5 years ago by ncohen

  • Status changed from needs_work to needs_review

comment:33 in reply to: ↑ 31 ; follow-up: Changed 5 years ago by 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.

comment:34 Changed 5 years ago by git

  • Commit changed from 1cbe479fd2a69d436fce8880d40275e41aaeb36b to 45aadab79b2eceae52a6ade5575848304f1a4048

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

45aadabtrac #17549: Review

comment:35 in reply to: ↑ 33 Changed 5 years ago by ncohen

That's what I meant.

Done.

Nathann

comment:36 Changed 5 years ago by ncohen

Is there anything else Jeroen ?

comment:37 follow-ups: Changed 5 years ago by 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...

comment:38 Changed 5 years ago by jdemeyer

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

comment:39 Changed 5 years ago by 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.

comment:40 in reply to: ↑ 37 Changed 5 years ago by 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.

comment:41 in reply to: ↑ 37 Changed 5 years ago by 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.)

comment:42 Changed 5 years ago by git

  • Commit changed from 45aadab79b2eceae52a6ade5575848304f1a4048 to 0ebf12ac4f21f78ebe2078b910624c1e97e3512d

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

0ebf12atrac #17549: Review

comment:43 follow-up: Changed 5 years ago by 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

comment:44 in reply to: ↑ 43 ; follow-up: Changed 5 years ago by 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.

comment:45 in reply to: ↑ 44 Changed 5 years ago by 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

comment:46 Changed 5 years ago by jdemeyer

  • 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 5 years ago by jdemeyer

  • Commit changed from 0ebf12ac4f21f78ebe2078b910624c1e97e3512d to e6da3d75f350ae019ba27070ff20e4df3943033a
  • Status changed from needs_review to positive_review

New commits:

e6da3d7Typo

comment:48 follow-up: Changed 5 years ago by jdemeyer

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

comment:49 in reply to: ↑ 48 Changed 5 years ago by ncohen

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

Oh, right.... O_o

Thanks for the review !

Nathann

comment:50 follow-up: Changed 5 years ago by jhpalmieri

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 5 years ago by ncohen

  • 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:

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

comment:52 Changed 5 years ago by jhpalmieri

  • Status changed from needs_review to positive_review

Great, thanks!

comment:53 Changed 5 years ago by vbraun

  • Branch changed from public/17549 to 19d6ff27e6e2542143dbe546968bdca198323d4c
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.