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 )
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:
Attachments (2)
Change History (27)
comment:1 Changed 7 years ago by
- Status changed from new to needs_review
comment:2 Changed 7 years ago by
comment:3 Changed 7 years ago by
- 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.
Changed 7 years ago by
comment:4 Changed 7 years ago by
- 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: ↓ 6 Changed 7 years ago by
- 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
- 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
- Status changed from needs_review to positive_review
comment:8 Changed 7 years ago by
- Description modified (diff)
comment:9 follow-up: ↓ 11 Changed 7 years ago by
- 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 thanSequence
, 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
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
- 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: ↓ 13 Changed 7 years ago by
- 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
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
- 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"
comment:15 follow-up: ↓ 17 Changed 7 years ago by
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
- 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
- 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: ↓ 20 Changed 7 years ago by
- 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
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
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
- I look at this ticket a bit more with an eye to merging it as-is.
- I add some documentation on #12688 extending this change.
comment:21 Changed 7 years ago by
- 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
Documentation is going up on #12688 (wrong ticket on comment 20, which I just edited).
comment:23 Changed 7 years ago by
- Keywords rd2 added
comment:24 Changed 7 years ago by
Thank you! And thanks for pointing out that comments are editable now!
comment:25 Changed 7 years ago by
- Merged in set to sage-5.0.beta10
- Resolution set to fixed
- Status changed from positive_review to closed
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:
Running tests right now.