Opened 9 years ago
Closed 9 years ago
#14867 closed enhancement (fixed)
Improve PARI qfminim documentation
Reported by: | Charlie Turner | Owned by: | Justin Walker |
---|---|---|---|
Priority: | minor | Milestone: | sage-6.2 |
Component: | interfaces | Keywords: | quadratic forms |
Cc: | John Cremona | Merged in: | |
Authors: | John Cremona, Charlie Turner, Jeroen Demeyer | Reviewers: | Robert Miller, Jeroen Demeyer, John Cremona |
Report Upstream: | N/A | Work issues: | |
Branch: | u/jdemeyer/ticket/14867 (Commits, GitHub, GitLab) | Commit: | 6e0f0d265a13c3ae1f320a2710268171d7c5f5bc |
Dependencies: | #15760 | Stopgaps: |
Description (last modified by )
Attachments (1)
Change History (23)
comment:1 Changed 9 years ago by
Description: | modified (diff) |
---|
comment:2 Changed 9 years ago by
comment:3 Changed 9 years ago by
Authors: | Charlie Turner → John Cremona, Charlie Turner |
---|
comment:4 Changed 9 years ago by
Component: | quadratic forms → interfaces |
---|---|
Description: | modified (diff) |
Keywords: | quadratic forms added |
Status: | new → needs_review |
As second author, I corrected the formatting of the docstring (so running sage -docbuild reference html gives no warnings) and also added some examples.
comment:5 follow-up: 6 Changed 9 years ago by
All tests pass when applied against sage-5.10.
My one nitpick is that the documentation uses both "length" and "square norm" to describe the role of B.
comment:6 Changed 9 years ago by
Replying to rlm:
All tests pass when applied against sage-5.10.
My one nitpick is that the documentation uses both "length" and "square norm" to describe the role of B.
Good point. I changed both to just "norm" which seems clear and unambiguous. Corrected patch in a second.
comment:7 Changed 9 years ago by
Reviewers: | → Robert Miller |
---|---|
Status: | needs_review → positive_review |
Looks good!
comment:9 Changed 9 years ago by
Status: | positive_review → needs_work |
---|
On 32-bit systems:
sage -t --long devel/sage/sage/libs/pari/gen.pyx ********************************************************************** File "devel/sage/sage/libs/pari/gen.pyx", line 8362, in sage.libs.pari.gen.gen.qfminim Failed example: pari(A.change_ring(RR)).qfminim(5,max=5,flag=2).python() Expected: [ [ -5 -10 -2 -7 3] [ 1 2 1 2 0] 10, 5.0000000002328306436538696289062500000, [ 1 2 0 1 -1] ] Got: [ [ -5 -10 -2 -7 3] [ 1 2 1 2 0] 10, 5.00000000023283064, [ 1 2 0 1 -1] ] **********************************************************************
comment:10 Changed 9 years ago by
This will be easy to fix by having separate 32/64-bit outputs as with numerous other pari library tests.
comment:11 Changed 9 years ago by
Description: | modified (diff) |
---|---|
Summary: | Pari's qfminim (flag = 1 or 2) does not allow you to output all vectors. (also documentation improvement) → Improve PARI qfminim documentation |
comment:12 Changed 9 years ago by
Dependencies: | → #15760 |
---|
comment:13 Changed 9 years ago by
Branch: | → u/jdemeyer/ticket/14867 |
---|---|
Created: | Jul 8, 2013, 4:04:40 PM → Jul 8, 2013, 4:04:40 PM |
Modified: | Jan 29, 2014, 4:02:09 PM → Jan 29, 2014, 4:02:09 PM |
comment:14 Changed 9 years ago by
Commit: | → 6e0f0d265a13c3ae1f320a2710268171d7c5f5bc |
---|---|
Status: | needs_work → needs_review |
comment:15 Changed 9 years ago by
Reviewers: | Robert Miller → Robert Miller, Jeroen Demeyer |
---|
I mostly just applied the documentation changes from trac_14867_qfminim1.patch (the code was already changed in #15760) and made a few more changes (you could consider these as reviewer changes).
comment:16 Changed 9 years ago by
Milestone: | sage-6.1 → sage-6.2 |
---|
comment:18 follow-up: 20 Changed 9 years ago by
I'm looking at it. The chenges look good but I cannot checkout the branch for some reason:
$ ./sage -dev checkout --ticket=u/jdemeyer/ticket/14867 Ticket name "u/jdemeyer/ticket/14867" is not valid or ticket does not exist on trac.
This is the usual way I check out tickets to reivew and worked before. I have not yet tried a manual git checkout.
comment:19 Changed 9 years ago by
Manual checkout worked fine, testing now. Perhaps I should report that failure of sage -dev.
comment:20 Changed 9 years ago by
Replying to cremona:
I'm looking at it. The chenges look good but I cannot checkout the branch for some reason:
$ ./sage -dev checkout --ticket=u/jdemeyer/ticket/14867 Ticket name "u/jdemeyer/ticket/14867" is not valid or ticket does not exist on trac.
For the record: that should have been
./sage -dev checkout --ticket=14867
This is the usual way I check out tickets to reivew and worked before. I have not yet tried a manual git checkout.
comment:21 Changed 9 years ago by
Authors: | John Cremona, Charlie Turner → John Cremona, Charlie Turner, Jeroen Demeyer |
---|---|
Reviewers: | Robert Miller, Jeroen Demeyer → Robert Miller, Jeroen Demeyer, John Cremona |
Status: | needs_review → positive_review |
Looks good to me.
comment:22 Changed 9 years ago by
Resolution: | → fixed |
---|---|
Status: | positive_review → closed |
Can you edit the docstring's INPUT section so that all the input parameters are listed (together with what they mean), and have an OUTPUT section after that? Currently they are a bit mixed up. (Surely the meaning of input param. B is that all vectors of norm <= B are returned, not =B?)
Also the indentation looks wrong in the changed line of code.
I'll come back and review this again when these are fixed!