Opened 8 years ago

Closed 8 years ago

#14867 closed enhancement (fixed)

Improve PARI qfminim documentation

Reported by: cturner Owned by: justin
Priority: minor Milestone: sage-6.2
Component: interfaces Keywords: quadratic forms
Cc: 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 cremona 8 years ago.
Replaces previous

Download all attachments as: .zip

Change History (23)

comment:1 Changed 8 years ago by cturner

  • Description modified (diff)

comment:2 Changed 8 years ago by 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 8 years ago by cturner

  • Authors changed from Charlie Turner to John Cremona, Charlie Turner

comment:4 Changed 8 years ago by cremona

  • Component changed from quadratic forms to interfaces
  • Description modified (diff)
  • Keywords quadratic forms added
  • Status changed from new to 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: Changed 8 years ago by 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.

comment:6 in reply to: ↑ 5 Changed 8 years ago by 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 8 years ago by cremona

Replaces previous

comment:7 Changed 8 years ago by rlm

  • Reviewers set to Robert Miller
  • Status changed from needs_review to positive_review

Looks good!

comment:8 Changed 8 years ago by davidloeffler

Apply trac_14867_qfminim1.patch​

(for patchbot)

comment:9 Changed 8 years ago by jdemeyer

  • Status changed from positive_review to 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 8 years ago by cremona

This will be easy to fix by having separate 32/64-bit outputs as with numerous other pari library tests.

comment:11 Changed 8 years ago by jdemeyer

  • Description modified (diff)
  • Summary changed from Pari's qfminim (flag = 1 or 2) does not allow you to output all vectors. (also documentation improvement) to Improve PARI qfminim documentation

comment:12 Changed 8 years ago by jdemeyer

  • Dependencies set to #15760

comment:13 Changed 8 years ago by jdemeyer

  • Branch set to u/jdemeyer/ticket/14867
  • Created changed from 07/08/13 16:04:40 to 07/08/13 16:04:40
  • Modified changed from 01/29/14 16:02:09 to 01/29/14 16:02:09

comment:14 Changed 8 years ago by jdemeyer

  • Commit set to 6e0f0d265a13c3ae1f320a2710268171d7c5f5bc
  • Status changed from needs_work to needs_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 8 years ago by jdemeyer

  • Reviewers changed from Robert Miller to 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 8 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:17 Changed 8 years ago by jdemeyer

Any reviewers please?

comment:18 follow-up: Changed 8 years ago by 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 8 years ago by cremona (previous) (diff)

comment:19 Changed 8 years ago by cremona

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

comment:20 in reply to: ↑ 18 Changed 8 years ago by 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 8 years ago by cremona

  • Authors changed from John Cremona, Charlie Turner to John Cremona, Charlie Turner, Jeroen Demeyer
  • Reviewers changed from Robert Miller, Jeroen Demeyer to Robert Miller, Jeroen Demeyer, John Cremona
  • Status changed from needs_review to positive_review

Looks good to me.

comment:22 Changed 8 years ago by vbraun

  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.