Remove Sequence test in span

[novoselt]

In Sage-5.0.beta4:

sage: span({0:vector([0,1])}, QQ)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)

/home/novoselt/Dropbox/Research/The 14th Case (personal copy)/<ipython console> in <module>()

/home/novoselt/sage-5.0.beta4/local/lib/python2.7/site-packages/sage/modules/free_module.pyc in span(gens, base_ring, check, already_echelonized)
    518         else:
    519             M = x.parent()
--> 520         return M.span(gens=gens, base_ring=base_ring, check=check, already_echelonized=already_echelonized)
    521 
    522 ###############################################################################

/home/novoselt/sage-5.0.beta4/local/lib/python2.7/site-packages/sage/modules/free_module.pyc in span(self, gens, base_ring, check, already_echelonized)
   2553         if is_FreeModule(gens):
   2554             gens = gens.gens()
-> 2555         if not isinstance(gens, (list, tuple, Sequence)):
   2556             raise TypeError, "Argument gens (= %s) must be a list, tuple, or sequence."%gens
   2557         if base_ring is None or base_ring == self.base_ring():

TypeError: isinstance() arg 2 must be a class, type, or tuple of classes and types

Such a command is likely to be a mistake, but the error message is irrelevant and is triggered by using a function name Sequence in isinstance. The attached patch removes this test as sequences are instances of lists and are correctly recognized anyway.

Apply:

attachment: trac_12541_decrease_span_typechecking.patch

CC: @rbeezer

Component: linear algebra

Keywords: rd2

Author: Andrey Novoseltsev

Reviewer: Rob Beezer

Merged: sage-5.0.beta10

Issue created by migration from https://trac.sagemath.org/ticket/12541

[rbeezer]

comment:2

Some linear algebra results come back as Sequence objects, so maybe that's the reason. But you are right, it doesn't seem to matter. With patch:

sage: A = matrix(QQ, 5, range(25))
sage: eigen = A.eigenvectors_right()
sage: basis = eigen[0][1]
sage: basis
[
(1, 0, 0, -4, 3),
(0, 1, 0, -3, 2),
(0, 0, 1, -2, 1)
]
sage: type(basis)
<class 'sage.structure.sequence.Sequence_generic'>
sage: U = span(basis)
sage: U
Vector space of degree 5 and dimension 3 over Rational Field
Basis matrix:
[ 1  0  0 -4  3]
[ 0  1  0 -3  2]
[ 0  0  1 -2  1]

Running tests right now.

[rbeezer]

comment:3

Looks like the new test is missing one line of output. Doesn't it need a starting line for the error message "Traceback...? The new test is failing for me in 5.0.beta1. Otherwise, I think it looks good.

[novoselt]

Attachment: trac_12541_remove_isinstance_Sequence_in_span.patch.gz

[novoselt]

comment:4

It certainly does! While fixing it, I noticed the same construction in another place, so I fixed it everywhere in this file.

[novoselt]

comment:5

On a related note: how about I will try to rewrite these functions in such a way, that they are happy with any iterable? I have a class at #11400 which wraps tuple without inheritance and it would be nice if it worked with spans, but adding another class to isinstance does not feel like a great idea...

[rbeezer]

comment:6

Replying to @novoselt:

On a related note: how about I will try to rewrite these functions in such a way, that they are happy with any iterable?

That sounds like a good idea. I've never been too excited about Sequence, I think it does a lot of work to get a common parent (which can be useful, but is often overkill).

I'm going to give this a positive review, it passes all tests. Documentation looks good (except on my 5.0.beta1 I don't have the new trac/sphinx role). So maybe further improvements can go on another ticket? If so, cc me on it, even though things are going to get hectic again for me.

Rob

[rbeezer]

Reviewer: Rob Beezer

[novoselt]

comment:9

Sorry Rob, how about this alternative patch? It affects mostly the same lines, but instead of removing Sequence gets rid of this isinstance completely. My reasoning for this:

• the present tests are wrong as stated in the description: the condition is either True or raises a confusing exception, not the intended one;
• the same check is repeated all over the place in functions that do nothing with the input except passing it further down - this makes it difficult to get the real processing place and figure out what has to be done;
• isinstance in general is not encouraged in Python - in my situation I have a perfectly good class resembling a tuple and working flawlessly if I get rid of these checks (in fact, PointCollection can be more suitable as a container for bases of modules than Sequence, although I don't intend to push this direction);
• totally wrong input still will produce some exception, e.g. using dictionaries gives "no span for integers" message;
• all tests pass with this alternative patch.

If you agree with my reasons, I'd rather use this solution.

[novoselt]

comment:10

I've posted my current work in progress at #12544 - to make it work, I either need the alternative patch here, or tweak FreeModule to recognize PointCollection and treat it as tuple, or write things like span(tuple(cone.rays())) in toric code, which mitigates the point of cone.rays() returning an "almost tuple"...

[rbeezer]

comment:11

Replying to @novoselt:

If you agree with my reasons, I'd rather use this solution.

Understood. Super-busy for a few days. Feel free to ping me if this slips, but I'll try to look at it later this week.

Rob

[loefflerd]

comment:12

Ping! Do I understand correctly that there's a consensus to use this second patch, but that it hasn't yet been reviewed?

Apply trac_12541_decrease_span_typechecking.patch

(for the patchbot).

[novoselt]

comment:13

Replying to @loefflerd:

Ping! Do I understand correctly that there's a consensus to use this second patch, but that it hasn't yet been reviewed?

Yes I want to merge the second patch, feel free to review it ;-) It should be easy, provided that you agree with the idea of the change. To see the problems that this patch solves, you can play with #12544 without this one.

[rbeezer]

comment:14

I find the error message in the doctest still somewhat cryptic. Here's a simplified explanation of what happens:

sage: span(QQ, [0])            
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<snip>
AttributeError: 'sage.rings.integer_ring.IntegerRing_class' object has no attribute 'span'

It appears that the span() function in free_module.pyx mostly tries to find an appropriate parent for the generators, and then hands everything off to the .span() method of the parent.

So here, the integer zero is construed to be the first generator, and the class of integers has no span method.

What do you think of checking if the first generator is a list, or if it is a free module element (is_FreeModuleElement), before proceeding with a supposed parent? This would allow an informative error message, such as - "generators must be lists of ring elements, or free module elements"

[novoselt]

Now with a doctest

[novoselt]

comment:15

Attachment: trac_12541_decrease_span_typechecking.patch.gz

OK, I've rewritten span function a bit to catch more issues and eliminate isinstance. Seems to work fine although I didn't run full tests yet.

[novoselt]

comment:16

All tests pass on beta8 with the new version!

[rbeezer]

comment:17

Replying to @novoselt:

OK, I've rewritten span function a bit to catch more issues and eliminate isinstance.

I like the looks of the new version. I did discover one problem.

sage: V = span(ZZ, [[1,2,3/4]])
sage: V
Free module of degree 3 and rank 1 over Integer Ring
Echelon basis matrix:
[  1   2 3/4]

I think this can be fixed by passing R into the final span method in the return (rather than base_ring.) This should have been caught by the span method, which seems to be the real fault here, I've isolated that at #12688.

I might write a few more doctests for the span() function as part of final testing for this.

[novoselt]

comment:18

I knew about this behaviour and was sure that this is intended. What we have in your example is a an integral span of elements of a rational vector space. I suspect I may have even used it somewhere. So if you think that it has to be changed, perhaps we should discuss it on sage-devel first.

In any case it seems orthogonal to this ticket which does the fix that I need for #12544, improves error messages from span, and did not affect the above behaviour. So I still propose merging the current version ;-)

[novoselt]

comment:19

The first test in the documentation actually shows integral span of rational vectors, so I really think that this was intended...

[rbeezer]

comment:20

Replying to @novoselt:

I knew about this behaviour and was sure that this is intended. What we have in your example is a an integral span of elements of a rational vector space. I suspect I may have even used it somewhere. So if you think that it has to be changed, perhaps we should discuss it on sage-devel first.

OK, that makes sense, and I see the test that allows this. Misunderstanding on my part.

But something more explicit in the documentation about this would help. How about

1. I look at this ticket a bit more with an eye to merging it as-is.
2. I add some documentation on Improve documentation of span method #12688 extending this change.

[rbeezer]

comment:21

Passes all tests and is a big improvement. So positive review.

There are still a few scenarios which give semi-cryptic error messages, so I think there is still room for impovement in this constructor.

[rbeezer]

comment:22

Documentation is going up on #12688 (wrong ticket on comment 20, which I just edited).

[rbeezer]

Changed keywords from none to rd2

[novoselt]

comment:24

Thank you! And thanks for pointing out that comments are editable now!

[jdemeyer]

Merged: sage-5.0.beta10