Opened 10 years ago

Closed 5 years ago

#13514 closed defect (fixed)

The documentation of representative_vector_list for Quadratic Forms should mention that it is for positive definite quadratic forms only

Reported by: mstreng Owned by: justin
Priority: trivial Milestone: sage-8.0
Component: quadratic forms Keywords: positive definite documentation quadratic form representative_vector_list
Cc: Merged in:
Authors: Frédéric Chapoton Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 7bd1701 (Commits, GitHub, GitLab) Commit: 7bd170182ca878f41ed27b68939c0d3f62728c79
Dependencies: Stopgaps:

Status badges

Description

On ​sage-nt, it was noted that it would be helpful if the documentation of representative_vector_list for quadratic forms mentions the fact that this function is for positive definite quadratic forms only, and that it returns random PariErrors otherwise.

Change History (15)

comment:1 Changed 9 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:2 Changed 8 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:3 Changed 8 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:4 Changed 8 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:5 Changed 5 years ago by chapoton

  • Authors set to Frédéric Chapoton
  • Branch set to u/chapoton/13514
  • Commit set to 3af1062f92cf206694f86a2de49bac9a4ab66bbe
  • Milestone changed from sage-6.4 to sage-8.0
  • Status changed from new to needs_review

done, please review

New commits:

​3af1062doc correction in quadratic forms

comment:6 Changed 5 years ago by git

  • Commit changed from 3af1062f92cf206694f86a2de49bac9a4ab66bbe to ee00e4ae1b1b0e2fa81068be14af8b9e7e85184f

Branch pushed to git repo; I updated commit sha1. New commits:

​ee00e4aMerge branch 'u/chapoton/13514' in 8.0.b0

comment:7 Changed 5 years ago by tscrim

I don't understand why you changed the default value; it seems to unnecessarily complicate things. Also, do we want to run that check rather than letting PARI give the error (possibly catching and sending an appropriate Sage error)?

comment:8 Changed 5 years ago by git

  • Commit changed from ee00e4ae1b1b0e2fa81068be14af8b9e7e85184f to 821eb493806bc40ac280395316b7f22e0c0728dd

Branch pushed to git repo; I updated commit sha1. New commits:

​821eb49trac 13514 change back default value of maxvectors

comment:9 Changed 5 years ago by chapoton

I changed back the default value, but did not modify the handling of errors.

comment:10 Changed 5 years ago by tscrim

More towards bikeshedding, but I don't think it should it be a TypeError. I feel more like it should be a NotImplementedError, since it is more like a feature that is lacking. Or perhaps a ValueError since it is based on the values of the matrix.

However, the PARI error that is raised seems fairly descriptive: 

sage: R = QuadraticForm(ZZ,2,[-4,-3,0])
sage: R.representation_vector_list(10)
...
PariError: domain error in minim0: form is not positive definite

Thus, I'm not completely convinced about doing a check on the Sage side, which seems to be done on the PARI side too.

Also, changing the error type might break some code. Although the chances of this are unlikely, so that is a very weak argument.

comment:11 Changed 5 years ago by git

  • Commit changed from 821eb493806bc40ac280395316b7f22e0c0728dd to 7bd170182ca878f41ed27b68939c0d3f62728c79

Branch pushed to git repo; I updated commit sha1. New commits:

​7bd1701trac 13514 back to PariError

comment:12 Changed 5 years ago by chapoton

done, put back the PariError

comment:13 Changed 5 years ago by chapoton

green bot, please review

comment:14 Changed 5 years ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

Thanks.

comment:15 Changed 5 years ago by vbraun

  • Branch changed from u/chapoton/13514 to 7bd170182ca878f41ed27b68939c0d3f62728c79
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.