Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

new doctest for french book about Sage #9395

Closed
zimmermann6 opened this issue Jun 30, 2010 · 28 comments
Closed

new doctest for french book about Sage #9395

zimmermann6 opened this issue Jun 30, 2010 · 28 comments

Comments

@zimmermann6
Copy link

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.

CC: @williamstein @JohnCremona @sagetrac-ylchapuy

Component: doctest coverage

Author: Paul Zimmermann, Yann Laigle-Chapuy

Reviewer: Yann Laigle-Chapuy, Paul Zimmermann

Merged: sage-4.6.alpha1

Issue created by migration from https://trac.sagemath.org/ticket/9395

@zimmermann6
Copy link
Author

comment:2

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.

@sagetrac-mvngu
Copy link
Mannequin

sagetrac-mvngu mannequin commented Jun 30, 2010

comment:3

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.

@zimmermann6
Copy link
Author

comment:4

Replying to @sagetrac-mvngu:

the new patch addresses your remarks.

Paul

@zimmermann6
Copy link
Author

Attachment: intro.pdf.gz

table of contents of the book

@zimmermann6
Copy link
Author

current version of chapter on number theory

@zimmermann6
Copy link
Author

comment:5

Attachment: numbertheory.pdf.gz

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!

@zimmermann6
Copy link
Author

comment:6

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

@williamstein
Copy link
Contributor

comment:7

Replying to @zimmermann6:

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

@zimmermann6
Copy link
Author

comment:8

Replying to @williamstein:

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

@sagetrac-ylchapuy
Copy link
Mannequin

sagetrac-ylchapuy mannequin commented Aug 15, 2010

comment:9

Replying to @zimmermann6:

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

@zimmermann6
Copy link
Author

Author: Paul Zimmermann

@zimmermann6
Copy link
Author

comment:10

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

@zimmermann6
Copy link
Author

Attachment: trac_9395.patch.gz

@sagetrac-ylchapuy
Copy link
Mannequin

sagetrac-ylchapuy mannequin commented Sep 1, 2010

apply on top of trac_9395.patch

@sagetrac-ylchapuy
Copy link
Mannequin

sagetrac-ylchapuy mannequin commented Sep 1, 2010

Reviewer: Yann Laigle-Chapuy

@sagetrac-ylchapuy
Copy link
Mannequin

sagetrac-ylchapuy mannequin commented Sep 1, 2010

comment:11

Attachment: trac_9395_review.patch.gz

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

@zimmermann6
Copy link
Author

Changed reviewer from Yann Laigle-Chapuy to Yann Laigle-Chapuy, Paul Zimmermann

@zimmermann6
Copy link
Author

Changed author from Paul Zimmermann to Paul Zimmermann, Yann Laigle-Chapuy

@zimmermann6
Copy link
Author

comment:12

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

yes I agree.
Paul

@qed777
Copy link
Mannequin

qed777 mannequin commented Sep 5, 2010

comment:13

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

@qed777 qed777 mannequin added s: needs work and removed s: positive review labels Sep 5, 2010
@zimmermann6
Copy link
Author

comment:14

Replying to @qed777:

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

@qed777
Copy link
Mannequin

qed777 mannequin commented Sep 6, 2010

comment:15

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?

@zimmermann6
Copy link
Author

apply only this patch

@zimmermann6
Copy link
Author

comment:16

Attachment: trac_9395_combined.patch.gz

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

done.
Paul

@qed777
Copy link
Mannequin

qed777 mannequin commented Sep 15, 2010

Merged: sage-4.6.alpha1

@qed777 qed777 mannequin removed the s: positive review label Sep 15, 2010
@qed777 qed777 mannequin closed this as completed Sep 15, 2010
@qed777
Copy link
Mannequin

qed777 mannequin commented Sep 17, 2010

Attachment: trac_9395-manifest_and_setup.patch.gz

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

@qed777
Copy link
Mannequin

qed777 mannequin commented Sep 17, 2010

comment:18

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

@qed777
Copy link
Mannequin

qed777 mannequin commented Sep 19, 2010

comment:19

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants