Opened 11 years ago
Closed 10 years ago
#10364 closed enhancement (fixed)
Vector constructor improvements
Reported by: | rbeezer | Owned by: | jason, was |
---|---|---|---|
Priority: | minor | Milestone: | sage-4.6.2 |
Component: | linear algebra | Keywords: | |
Cc: | jason | Merged in: | sage-4.6.2.alpha0 |
Authors: | Rob Beezer | Reviewers: | Andrey Novoseltsev |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description
Patch creates a zero_vector
constructor, mirroring the zero_matrix
constructor. It adds error-checking to the vector
constructor for the case of a negative degree.
Finally, it adds a new, more informative error message when a dictionary is used for the entries of a (sparse) vector and a degree is given. Presently, the following syntax executes without an error, though the result would not be what we should expect. This "works" since the dictionary has "length" 3.
sage: vector(QQ, 3, {0:0, 2:5, 12:-1}) (0, 0, 5, 0, 0, 0, 0, 0, 0, 0, 0, 0, -1)
This could be improved further by adjusting the prepare_dict
method to accept an optional integer degree for the number of entries. Then instead of an error we could have the (fictional) behavior:
sage: vector(QQ, 6, {1:2, 4:-3}) (0, 2, 0, 0, -3, 0)
This depends on #10362 since one doctest there has changed in this patch.
Attachments (2)
Change History (17)
Changed 11 years ago by
comment:1 Changed 11 years ago by
- Status changed from new to needs_review
comment:2 Changed 11 years ago by
- Cc jason added
comment:3 Changed 11 years ago by
- Status changed from needs_review to needs_info
comment:4 follow-up: ↓ 5 Changed 11 years ago by
Giving the degree and giving the entries as a dictionary gives an error (or an unexpected result) in the unpatched state and probably has for a long time. The problem is that you could always give a degree, though this was totally undocumented. When a degree is given, it checks for the length of the entries. For a dictionary this goes boom, or coincidentally passes this check and behaves poorly. On #10362 I was trying to mostly document the degree possibility.
Current patch makes it so that a degree/dictionary call will *always* give an exception. So it is an improvement. I agree 100% that your suggestion is the *right* thing to do long-term, and I can see where and how to do it.
However, I am on a mission to clean up as much of the linear algebra code that is wrong or missing and trying not to make any one ticket too involved (explanation on sage-devel soon). I'd be happy to document the current behavior and an outline of a fix (your ideas on exceptions are perfect) on another ticket. Would it be OK if I moved this to another ticket?
Thanks, Rob
comment:5 in reply to: ↑ 4 ; follow-up: ↓ 14 Changed 11 years ago by
- Status changed from needs_info to needs_review
Replying to rbeezer:
However, I am on a mission to clean up as much of the linear algebra code that is wrong or missing and trying not to make any one ticket too involved (explanation on sage-devel soon). I'd be happy to document the current behavior and an outline of a fix (your ideas on exceptions are perfect) on another ticket. Would it be OK if I moved this to another ticket?
Sure, let it be another ticket. I thought that it is an easy change but I didn't actually look at the relevant code.
comment:6 follow-up: ↓ 7 Changed 11 years ago by
I don't quite like error-checking in zero_vector
because I think that it belongs to the general vector constructor. I think code like
def zero_vector(arg0, arg1=None): if arg1 is None: arg0, arg1 = ZZ, arg0 return vector(arg0, arg1)
would be better for uniform input handling. (Maybe it would even make sense to get zero integer vector from vector(5)
, but that's a bigger change.)
I guess also that the list in CALL FORMAT has the issue with INPUT right under it as on #10362.
comment:7 in reply to: ↑ 6 Changed 11 years ago by
- Status changed from needs_review to needs_info
Replying to novoselt:
I don't quite like error-checking in
zero_vector
because I think that it belongs to the general vector constructor.
I agree that using the error-checking code from the more general vector
constructor would be best. But the vector()
constructor is so permissive and accepts such a variety of input, that I think any input to a new zero_vector
*must* get scrutinized (and then report the problem). Here's the example and test:
def mock_zero_vector(arg0, arg1=None): if arg1 is None: arg0, arg1 = sage.rings.integer_ring.IntegerRing(), arg0 return vector(arg0, arg1)
sage: v = sage.modules.free_module_element.mock_zero_vector([1,2,3]) sage: v (1, 2, 3)
At least one check is being made in the general constructor - seeing if the proposed degree is negative.
I guess also that the list in CALL FORMAT has the issue with INPUT right under it as on #10362.
Yes, I'll get to this with any other changes. Thanks.
Rob
comment:8 follow-up: ↓ 9 Changed 11 years ago by
Hmmm, indeed, it is necessary to do extra checks to construct zero vectors instead of something else. Let it be as is then!
Changed 11 years ago by
comment:9 in reply to: ↑ 8 Changed 11 years ago by
- Status changed from needs_info to needs_review
Replying to novoselt:
Hmmm, indeed, it is necessary to do extra checks to construct zero vectors instead of something else. Let it be as is then!
Sounds good. Thanks.
Standalone v2 patch just fixes up the documentation - call formats and output block only. Thanks for your help with these.
Rob
comment:10 Changed 11 years ago by
- Reviewers set to Andrey Novoseltsev
- Status changed from needs_review to positive_review
The new patch looks good and passes all tests!
comment:11 Changed 11 years ago by
Please set milestone
comment:12 Changed 11 years ago by
- Milestone set to sage-4.6.1
comment:13 Changed 11 years ago by
- Milestone changed from sage-4.6.1 to sage-4.6.2
comment:14 in reply to: ↑ 5 Changed 11 years ago by
comment:15 Changed 10 years ago by
- Merged in set to sage-4.6.2.alpha0
- Resolution set to fixed
- Status changed from positive_review to closed
Why is it an error to specify the degree of a vector when entries are given as a dictionary? To me it makes more sense to give explicit degree in this case (and avoid unnatural addition of the "terminal element") rather than when entries are packed into a list. I propose that if the degree is given, a vector of this degree is constructed and if the dictionary contains any keys above the degree an exception is raised.