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: |
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
- Milestone changed from sage-5.11 to sage-5.12
comment:2 Changed 8 years ago by
- Milestone changed from sage-6.1 to sage-6.2
comment:3 Changed 8 years ago by
- Milestone changed from sage-6.2 to sage-6.3
comment:4 Changed 8 years ago by
- Milestone changed from sage-6.3 to sage-6.4
comment:5 Changed 5 years ago by
- 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
comment:6 Changed 5 years ago by
- Commit changed from 3af1062f92cf206694f86a2de49bac9a4ab66bbe to ee00e4ae1b1b0e2fa81068be14af8b9e7e85184f
Branch pushed to git repo; I updated commit sha1. New commits:
ee00e4a | Merge branch 'u/chapoton/13514' in 8.0.b0
|
comment:7 Changed 5 years ago by
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
- Commit changed from ee00e4ae1b1b0e2fa81068be14af8b9e7e85184f to 821eb493806bc40ac280395316b7f22e0c0728dd
Branch pushed to git repo; I updated commit sha1. New commits:
821eb49 | trac 13514 change back default value of maxvectors
|
comment:9 Changed 5 years ago by
I changed back the default value, but did not modify the handling of errors.
comment:10 Changed 5 years ago by
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
- Commit changed from 821eb493806bc40ac280395316b7f22e0c0728dd to 7bd170182ca878f41ed27b68939c0d3f62728c79
Branch pushed to git repo; I updated commit sha1. New commits:
7bd1701 | trac 13514 back to PariError
|
comment:12 Changed 5 years ago by
done, put back the PariError
comment:13 Changed 5 years ago by
green bot, please review
comment:14 Changed 5 years ago by
- Reviewers set to Travis Scrimshaw
- Status changed from needs_review to positive_review
Thanks.
comment:15 Changed 5 years ago by
- Branch changed from u/chapoton/13514 to 7bd170182ca878f41ed27b68939c0d3f62728c79
- Resolution set to fixed
- Status changed from positive_review to closed
done, please review
New commits:
doc correction in quadratic forms