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:

Status badges

Attachments (1)

trac_14867_qfminim1.patch (3.3 KB) - added by John Cremona 9 years ago.
Replaces previous

Download all attachments as: .zip

Change History (23)

comment:1 Changed 9 years ago by Charlie Turner

Description: modified (diff)

comment:2 Changed 9 years ago by John Cremona

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!

comment:3 Changed 9 years ago by Charlie Turner

Authors: Charlie TurnerJohn Cremona, Charlie Turner

comment:4 Changed 9 years ago by John Cremona

Component: quadratic formsinterfaces
Description: modified (diff)
Keywords: quadratic forms added
Status: newneeds_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 Changed 9 years ago by Robert Miller

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 in reply to:  5 Changed 9 years ago by John Cremona

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.

Changed 9 years ago by John Cremona

Attachment: trac_14867_qfminim1.patch added

Replaces previous

comment:7 Changed 9 years ago by Robert Miller

Reviewers: Robert Miller
Status: needs_reviewpositive_review

Looks good!

comment:8 Changed 9 years ago by David Loeffler

Apply trac_14867_qfminim1.patch​

(for patchbot)

comment:9 Changed 9 years ago by Jeroen Demeyer

Status: positive_reviewneeds_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 John Cremona

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 Jeroen Demeyer

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 Jeroen Demeyer

Dependencies: #15760

comment:13 Changed 9 years ago by Jeroen Demeyer

Branch: u/jdemeyer/ticket/14867
Created: Jul 8, 2013, 4:04:40 PMJul 8, 2013, 4:04:40 PM
Modified: Jan 29, 2014, 4:02:09 PMJan 29, 2014, 4:02:09 PM

comment:14 Changed 9 years ago by Jeroen Demeyer

Commit: 6e0f0d265a13c3ae1f320a2710268171d7c5f5bc
Status: needs_workneeds_review

New commits:

8c12459Speed up short_vector_list_up_to_length()
3e80bfdSome "long time" fixes in quadratic forms
6e0f0d2Improve PARI qfminim documentation

comment:15 Changed 9 years ago by Jeroen Demeyer

Reviewers: Robert MillerRobert 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 For batch modifications

Milestone: sage-6.1sage-6.2

comment:17 Changed 9 years ago by Jeroen Demeyer

Any reviewers please?

comment:18 Changed 9 years ago by John 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.

This is the usual way I check out tickets to reivew and worked before. I have not yet tried a manual git checkout.

Last edited 9 years ago by John Cremona (previous) (diff)

comment:19 Changed 9 years ago by John Cremona

Manual checkout worked fine, testing now. Perhaps I should report that failure of sage -dev.

comment:20 in reply to:  18 Changed 9 years ago by John Cremona

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 John Cremona

Authors: John Cremona, Charlie TurnerJohn Cremona, Charlie Turner, Jeroen Demeyer
Reviewers: Robert Miller, Jeroen DemeyerRobert Miller, Jeroen Demeyer, John Cremona
Status: needs_reviewpositive_review

Looks good to me.

comment:22 Changed 9 years ago by Volker Braun

Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.