Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#9395 closed enhancement (fixed)

new doctest for french book about Sage

Reported by: zimmerma Owned by: mvngu
Priority: major Milestone: sage-4.6
Component: doctest coverage Keywords:
Cc: was, cremona, ylchapuy Merged in: sage-4.6.alpha1
Authors: Paul Zimmermann, Yann Laigle-Chapuy Reviewers: Yann Laigle-Chapuy, Paul Zimmermann
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

The attached patch includes a new doctest for a book (in french) some people are writing about Sage (see the README file for the list of authors).

This book will be available under a CC-by-sa license.

This patch contains only the doctests for one chapter (about number theory). Some more doctests will follow, one per chapter, but we already submit that one to see if some things need to be fixed.

This doctest was tested successfully with Sage 4.4.2 under Fedora 12.

Attachments (6)

intro.pdf (90.8 KB) - added by zimmerma 7 years ago.
table of contents of the book
numbertheory.pdf (501.4 KB) - added by zimmerma 7 years ago.
current version of chapter on number theory
trac_9395.patch (5.3 KB) - added by zimmerma 7 years ago.
trac_9395_review.patch (1.5 KB) - added by ylchapuy 7 years ago.
apply on top of trac_9395.patch
trac_9395_combined.patch (5.8 KB) - added by zimmerma 7 years ago.
apply only this patch
trac_9395-manifest_and_setup.patch (1.0 KB) - added by mpatel 7 years ago.
Update MANIFEST.in and setup.py with new file and directory

Download all attachments as: .zip

Change History (25)

comment:1 Changed 7 years ago by zimmerma

  • Cc was cremona added

comment:2 Changed 7 years ago by zimmerma

  • Status changed from new to needs_review

I found no way so that the %timeit ... or time ... lines are tested, thus I've added # not tested. If somebody knows how to do so that in %timeit a = b + c at least the instruction a = b + c is performed, please tell me.

comment:3 follow-up: Changed 7 years ago by mvngu

  • Status changed from needs_review to needs_work

You don't need to put your doctests inside a function. I think it's much simpler to put your doctests inside a docstring. See the files under tests/ in the Sage library for examples. You should also consider giving your book's directory name a more descriptive name. Something like "number_theory_zimmermann", not just "sagebook". Or do you envision the directory "sagebook" to contain doctests of books that leverage the Sage doctesting framework? In that case, see this script for a proof of concept for automatic extraction of Sage code and doctesting the extracted code. That script has been tested on this Sage book.

comment:4 in reply to: ↑ 3 Changed 7 years ago by zimmerma

  • Status changed from needs_work to needs_review

Replying to mvngu:

the new patch addresses your remarks.

Paul

Changed 7 years ago by zimmerma

table of contents of the book

Changed 7 years ago by zimmerma

current version of chapter on number theory

comment:5 Changed 7 years ago by zimmerma

The version 1.0 of the book has now appeared, and is available from http://www.loria.fr/~zimmerma/sagebook.html. Any feedback is most welcome!

comment:6 follow-ups: Changed 7 years ago by zimmerma

  • Cc ylchapuy added

Yann, please could you review this? What you have to do (William please correct me if needed):

1) [optional] check the content of the numbertheory.py file matches the corresponding chapter of the book. I write "optional" since this should be our (the authors of the book) responsability.

2) check the new doctests pass with the latest Sage version (we used 4.4.4)

Paul

comment:7 in reply to: ↑ 6 ; follow-up: Changed 7 years ago by was

Replying to zimmerma:

Yann, please could you review this? What you have to do (William please correct me if needed):

Sounds good to me.

I've been advertising your book to all the French people I meet in Paris, e.g., at Euroscipy, and also to Bernandi at Jussieu today (he's one of the original PARI guys).

comment:8 in reply to: ↑ 7 Changed 7 years ago by zimmerma

Replying to was:

I've been advertising your book to all the French people I meet in Paris, e.g., at Euroscipy, and also to Bernandi at Jussieu today (he's one of the original PARI guys).

thanks, some people of my lab who were at Euroscipy indeed told me today that they heard of our book there! Paul

comment:9 in reply to: ↑ 6 Changed 7 years ago by ylchapuy

  • Status changed from needs_review to needs_info

Replying to zimmerma:

Yann, please could you review this? What you have to do (William please correct me if needed):

1) [optional] check the content of the numbertheory.py file matches the corresponding chapter of the book. I write "optional" since this should be our (the authors of the book) responsability.

2) check the new doctests pass with the latest Sage version (we used 4.4.4)

Paul

Sorry for the delay. All tests passed with Sage 4.5.2.

Regarding the timeit issue, you could change

    sage: %timeit (a^e) # not tested

for

    sage: timeit('a^e') # random

(and also # long for some of them).

I put it to need info for now, but feel free either to change this and ask me for a review (I'll be faster this time) or don't change anything and give a positive review.

Yann

comment:10 Changed 7 years ago by zimmerma

  • Authors set to Paul Zimmermann
  • Status changed from needs_info to needs_review

thank you Yann for your comments. I attach a new patch taking them into account. I left the last time r=... as not tested since I do not know how to make it work, without changing time into timeit, which would be not coherent with the book.

Paul

Changed 7 years ago by zimmerma

Changed 7 years ago by ylchapuy

apply on top of trac_9395.patch

comment:11 Changed 7 years ago by ylchapuy

  • Reviewers set to Yann Laigle-Chapuy

Everything is still ok for me. I added 'long time' to the longest tests, this reduces the time for normal testing to 12 seconds on my computer compared to 67 for the long version.

Paul, if you agree with my reviewer's patch, you can give this ticket a positive review.

Yann

comment:12 Changed 7 years ago by zimmerma

  • Authors changed from Paul Zimmermann to Paul Zimmermann, Yann Laigle-Chapuy
  • Reviewers changed from Yann Laigle-Chapuy to Yann Laigle-Chapuy, Paul Zimmermann
  • Status changed from needs_review to positive_review

Paul, if you agree with my reviewer's patch, you can give this ticket a positive review.

yes I agree. Paul

comment:13 follow-up: Changed 7 years ago by mpatel

  • Status changed from positive_review to needs_work

Could someone please update both patches with more descriptive commit strings (and change the status back to "positive review")?

comment:14 in reply to: ↑ 13 Changed 7 years ago by zimmerma

Replying to mpatel:

Could someone please update both patches with more descriptive commit strings (and change the status back to "positive review")?

done with a combined patch (apply only that one). Is that what you wanted?

Paul

comment:15 Changed 7 years ago by mpatel

I apologize for not being clearer. The first line of the commit string for each patch to be merged should start with the ticket number and contain a short description of what the patch does. This line should be < 80 characters in length.

For example: #9395: Number theory doctests for French book about Sage and #9395: Reviewer patch: tag longest tests as long.

The reason for these policies is so that hg log and hg log filename.py tell us what a changeset did and which Sage trac ticket we can consult for background.

Of course, extra lines are welcome and may help to explain details to a reviewer or someone who uses hg log -p.

Could you update the just first line of the combined patch?

Changed 7 years ago by zimmerma

apply only this patch

comment:16 Changed 7 years ago by zimmerma

  • Status changed from needs_work to positive_review

Could you update the just first line of the combined patch?

done. Paul

comment:17 Changed 7 years ago by mpatel

  • Merged in set to sage-4.6.alpha1
  • Resolution set to fixed
  • Status changed from positive_review to closed

Changed 7 years ago by mpatel

Update MANIFEST.in and setup.py with new file and directory

comment:18 Changed 7 years ago by mpatel

I've added a release manager's patch that ensures the files added here are included in a new Sage source distribution.

comment:19 Changed 7 years ago by mpatel

Ticket #9951, about a missing __init__.py file, follows up on this ticket.

Note: See TracTickets for help on using tickets.