Opened 7 years ago

Closed 7 years ago

#12541 closed defect (fixed)

Remove Sequence test in span

Reported by: novoselt Owned by: jason, was
Priority: minor Milestone: sage-5.0
Component: linear algebra Keywords: rd2
Cc: rbeezer Merged in: sage-5.0.beta10
Authors: Andrey Novoseltsev Reviewers: Rob Beezer
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by davidloeffler)

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:

trac_12541_decrease_span_typechecking.patch

Attachments (2)

trac_12541_remove_isinstance_Sequence_in_span.patch (3.2 KB) - added by novoselt 7 years ago.
trac_12541_decrease_span_typechecking.patch (7.6 KB) - added by novoselt 7 years ago.
Now with a doctest

Download all attachments as: .zip

Change History (27)

comment:1 Changed 7 years ago by novoselt

  • Status changed from new to needs_review

comment:2 Changed 7 years ago by rbeezer

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.

comment:3 Changed 7 years ago by rbeezer

  • Status changed from needs_review to needs_work

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.

comment:4 Changed 7 years ago by novoselt

  • Status changed from needs_work to needs_review

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

comment:5 follow-up: Changed 7 years ago by novoselt

  • Status changed from needs_review to needs_info

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...

comment:6 in reply to: ↑ 5 Changed 7 years ago by rbeezer

  • Reviewers set to Rob Beezer
  • Status changed from needs_info to needs_review

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

comment:7 Changed 7 years ago by rbeezer

  • Status changed from needs_review to positive_review

comment:8 Changed 7 years ago by rbeezer

  • Description modified (diff)

comment:9 follow-up: Changed 7 years ago by novoselt

  • Status changed from positive_review to needs_info

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.

comment:10 Changed 7 years ago by novoselt

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"...

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

  • Status changed from needs_info to needs_review

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

comment:12 follow-up: Changed 7 years ago by davidloeffler

  • Description modified (diff)

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).

comment:13 in reply to: ↑ 12 Changed 7 years ago by novoselt

Replying to davidloeffler:

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.

comment:14 Changed 7 years ago by rbeezer

  • Status changed from needs_review to needs_info

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"

Changed 7 years ago by novoselt

Now with a doctest

comment:15 follow-up: Changed 7 years ago by novoselt

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.

comment:16 Changed 7 years ago by novoselt

  • Status changed from needs_info to needs_review

All tests pass on beta8 with the new version!

comment:17 in reply to: ↑ 15 Changed 7 years ago by rbeezer

  • Status changed from needs_review to needs_work

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.

comment:18 follow-up: Changed 7 years ago by novoselt

  • Status changed from needs_work to needs_review

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 ;-)

comment:19 Changed 7 years ago by novoselt

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

comment:20 in reply to: ↑ 18 Changed 7 years ago by rbeezer

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 #12688 extending this change.
Last edited 7 years ago by rbeezer (previous) (diff)

comment:21 Changed 7 years ago by rbeezer

  • Status changed from needs_review to positive_review

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.

comment:22 Changed 7 years ago by rbeezer

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

comment:23 Changed 7 years ago by rbeezer

  • Keywords rd2 added

comment:24 Changed 7 years ago by novoselt

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

comment:25 Changed 7 years ago by jdemeyer

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