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:

Status badges

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)

trac_10364-zero-vector-constructor.patch (5.1 KB) - added by rbeezer 11 years ago.
trac_10364-zero-vector-constructor-v2.patch (5.1 KB) - added by rbeezer 11 years ago.

Download all attachments as: .zip

Change History (17)

Changed 11 years ago by rbeezer

comment:1 Changed 11 years ago by rbeezer

  • Status changed from new to needs_review

comment:2 Changed 11 years ago by jason

  • Cc jason added

comment:3 Changed 11 years ago by novoselt

  • Status changed from needs_review to needs_info

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.

comment:4 follow-up: Changed 11 years ago by rbeezer

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: Changed 11 years ago by novoselt

  • 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: Changed 11 years ago by novoselt

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 rbeezer

  • 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: Changed 11 years ago by novoselt

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 rbeezer

comment:9 in reply to: ↑ 8 Changed 11 years ago by rbeezer

  • 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 novoselt

  • 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 jdemeyer

Please set milestone

comment:12 Changed 11 years ago by novoselt

  • Milestone set to sage-4.6.1

comment:13 Changed 11 years ago by jdemeyer

  • Milestone changed from sage-4.6.1 to sage-4.6.2

comment:14 in reply to: ↑ 5 Changed 11 years ago by rbeezer

Replying to novoselt:

Sure, let it be another ticket. I thought that it is an easy change but I didn't actually look at the relevant code.

Degree plus dictionary enhancement is now on #10439.

comment:15 Changed 10 years ago by jdemeyer

  • Merged in set to sage-4.6.2.alpha0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.