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

Add quickstarts from PREP workshops to standard documentation #13381

Closed
kcrisman opened this issue Aug 19, 2012 · 32 comments
Closed

Add quickstarts from PREP workshops to standard documentation #13381

kcrisman opened this issue Aug 19, 2012 · 32 comments

Comments

@kcrisman
Copy link
Member

This ticket is to add the quickstart tutorials, originally at #13260. It was split off from that ticket for convenience.

Apply attachment: trac_13381-quickstarts.patch, attachment: trac_13381-ref.patch, and attachment: trac_13381-fixtests.patch.

Depends on #5457
Depends on #9005
Depends on #14014

CC: @jasongrout @rbeezer @dandrake @wdjoyner @sagetrac-mhampton @benjaminfjones @wypong @dcernst @sagetrac-travis @novoselt @jhpalmieri

Component: documentation

Author: Karl-Dieter Crisman

Reviewer: John Palmieri, Jason Grout, Jeroen Demeyer

Merged: sage-5.11.rc0

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

@kcrisman
Copy link
Member Author

Dependencies: #13260

@kcrisman
Copy link
Member Author

Based on 5.3.beta1

@kcrisman

This comment has been minimized.

@kcrisman
Copy link
Member Author

comment:2

Attachment: trac_13381-quickstarts.patch.gz

Ready for review.

Patchbot, apply trac_13381-quickstarts.patch.

@jhpalmieri

This comment has been minimized.

@jhpalmieri
Copy link
Member

comment:3

Here is a referee patch. If it's okay with you, then one item remains to be addressed: in NumAnalysis.rst:

  • "The first step is to normalize" - The first step in what? It's not clear what's being done here. It looks like you want to convert x to floating point binary, but it also looks like that was done a few lines earlier in x.str(base=2). So a little more explanation would be helpful.

Other comments (I think most of my changes are self-explanatory):

  • in Abstract-Algebra.rst and elsewhere, I've changed \ZZ_n to \ZZ/n\ZZ. The subscript notation is ambiguous — many people will read \ZZ_p as denoting the p-adics, not the integers mod p, and so in my opinion this notation should never be used to denote quotient rings of the integers.

  • in Linear-Algebra.rst, I've added a definition of "left kernel": personally, I can tell from the name "left kernel" that either the matrix or the vector is supposed to be on the left, but I can never remember which.

The use of upper case in the file names here (and in the parent directory) doesn't follow the same pattern as elsewhere in the Sage library. This is certainly not a big deal, but it looks a little strange to me.

@jhpalmieri
Copy link
Member

Attachment: trac_13381-ref.patch.gz

referee patch; apply on top of other one

@jasongrout
Copy link
Member

comment:4

regarding left vs. right: I guess I always think that the way I do it naturally is the "right" (correct) way to do it :). That helps me remember that the normal linear algebra way (column vectors, A*x) is the "right" way.

Not that I'm biased or anything :).

@jasongrout
Copy link
Member

comment:5

John, did you review this positively, or is there still more to do?

@jhpalmieri
Copy link
Member

comment:6

In my comment above, I said that "one item remains to be addressed". This is still true.

@kcrisman
Copy link
Member Author

kcrisman commented Oct 2, 2012

comment:7

I think he was at least thinking the NumAnalysis.rst thing about normalization needed to be fixed (which I think I agree about, but it was a while ago that I read this so I forget). Also, his patch is long enough that is probably could use a quick once-over to make sure he didn't miss any obvious formatting things, though this IS jhpalmieri, the king of Sphinx, we're talking about here :)

@jasongrout
Copy link
Member

comment:8

The heading could be changed to "Converting to IEEE floating point binary format"---does that make things clearer? The rest of the paragraph in question explains that the first step in IEEE conversion is normalizing the number so that it is a number between 1 and 2 (multiplied by an appropriate power of 2).

@jasongrout
Copy link
Member

comment:9

Or how about this rewording of the paragraph:

Now let's convert ``x`` to the IEEE 754 binary format that is commonly used in computers.  For `IEEE 754
<http://grouper.ieee.org/groups/754/>`_, the first step in getting the binary format is to normalize the 
number, or express the number as a number between 1 and 2 multiplied by a power of 2.  For our ``x`` above, 
we multiply by `2^{-3}` to get a number between 1 and 2.

Is that better?

@kcrisman
Copy link
Member Author

kcrisman commented Oct 2, 2012

Reviewer: John Palmieri, Jason Grout

@kcrisman
Copy link
Member Author

kcrisman commented Oct 2, 2012

comment:10

That seems reasonable. Actually, making the section heading "Converting to floating point binary (IEEE format)" would be the best compromise on the heading title as well.

@jhpalmieri
Copy link
Member

comment:11

These all look like good suggestions to me.

@kcrisman
Copy link
Member Author

Apply on top of others

@kcrisman

This comment has been minimized.

@kcrisman
Copy link
Member Author

Changed dependencies from #13260 to none

@kcrisman
Copy link
Member Author

comment:12

Attachment: trac_13381-ieee.patch.gz

Okay, I've finished this off by just implementing this. John was okay with this, and all I did was put what Jason suggested. Yay!

@jdemeyer
Copy link

comment:13

There are obvious doctest failures, why did this ever get positive review?

While you're at it, you should also use the new-style doctest continuations

sage: for K in D.conjugacy_classes_subgroups(): 
....:     print K 

instead of

sage: for K in D.conjugacy_classes_subgroups(): 
...       print K 

@jdemeyer
Copy link

Dependencies: #5457, #9005, #14014

@kcrisman
Copy link
Member Author

comment:14

There are obvious doctest failures, why did this ever get positive review?

Because the only previous complaints were about non-doctest issues. Patchbot is very flaky, in my opinion, and often fails for trivial reasons.

Interestingly, at least one of these failures indicates a regression in Sage. The rest are pretty easy to fix, I'll do this later today along with the new-style continuations.

@kcrisman
Copy link
Member Author

comment:15

I also changed to new continuations in the main prep stuff. Very annoying. Some of the changes are pretty recent as well.

There are two remaining issues, I think.

sage: import scipy.stats
sage: my_data=[lognormvariate(2,3) for i in range(10)]
sage: scipy.stats.scoreatpercentile(my_data, int(50))
1.2439846750720158
sage: scipy.stats.scoreatpercentile(my_data, 50)
---------------------------------------------------------------------------

TypeError: 'sage.rings.integer.Integer' object is not iterable

This is because scipy now assumes a Sage Integer is not a scalar. I'm not sure whether this is an upstream bug or not.

Then there is this strange regression in plotting power series.

sage: R.<t> = PowerSeriesRing(QQ)
sage: c = t + 1 + O(t^2)
sage: plot(c,(t,0,1))
AttributeError: 'float' object has no attribute 'parent'

If I just do the polynomial, it's fine, so I can fix the test. Interestingly, I get a different error with the standard syntax:

plot(c.polynomial(),(t,0,1))
Error...

@kcrisman
Copy link
Member Author

comment:16

I think I have discovered a really nasty bug with new-style line continuations. I cannot get live documentation to work with it. So I am not going to do this. For some reason the new such doctests don't seem to be in the live documentation (probably in underscore methods, and not a lot of them), so that's why it hasn't been noticed. This is as of 5.11.beta1.

@kcrisman
Copy link
Member Author

Changed reviewer from John Palmieri, Jason Grout to John Palmieri, Jason Grout, Jeroen Demeyer

@kcrisman

This comment has been minimized.

@kcrisman
Copy link
Member Author

comment:17

Patchbot, apply trac_13381-quickstarts.patch, trac_13381-ref.patch, and trac_13381-fixtests.patch

Doctests are now fixed, I fixed some broken links (no thanks to the reorganization of the reference manual!), and of course the IEEE business. All tests pass. There is nothing substantive changed, though, and I think jeroen's comments suffice.

@kcrisman
Copy link
Member Author

comment:18

Still some issues in live documentation, one moment...

@kcrisman
Copy link
Member Author

Apply last

@kcrisman
Copy link
Member Author

comment:19

Attachment: trac_13381-fixtests.patch.gz

All set!!

Patchbot, apply trac_13381-quickstarts.patch, trac_13381-ref.patch, and trac_13381-fixtests.patch

@jdemeyer
Copy link

Merged: sage-5.11.rc0

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

4 participants