#11751 closed defect (fixed)
make free_module_generic_pid also work for pid's other than integers
Reported by: | mderickx | Owned by: | jason, was |
---|---|---|---|
Priority: | major | Milestone: | sage-4.7.2 |
Component: | linear algebra | Keywords: | |
Cc: | Merged in: | sage-4.7.2.alpha3 | |
Authors: | Maarten Derickx, Julian Rueth | Reviewers: | Julian Rueth, Maarten Derickx |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
Before this patch the following code would fail
sage: R.<x> = QQ[] sage: L = R^1 sage: M = L.span([[1/x]]) sage: M([1/x])
Apply
- 11751_free_module_generic_pid-fix.patch
- trac_11751_free_module_generic_pid-review.patch
- trac_11751_whitespace.patch
to the Sage library.
Attachments (3)
Change History (21)
Changed 10 years ago by
comment:1 Changed 10 years ago by
- Description modified (diff)
comment:2 Changed 10 years ago by
- Description modified (diff)
comment:3 Changed 10 years ago by
comment:4 Changed 10 years ago by
A full output of the errors is shown in http://pastebin.com/4B4xmRDZ
comment:5 Changed 10 years ago by
It seems that the line entries = [R(x) for x in entries]
in coerce.pyx (roughly line 3540) is wrong. The components of a element in a free R module may not be in R at all. I'm working on a fix for that.
comment:6 Changed 10 years ago by
- Status changed from new to needs_review
comment:7 Changed 10 years ago by
The patch looks good :). Didn't try it yet how it works with doctests and yet but at least the code looks reasonable.
If this passes all test, then at least I want something like the following as a doctest:
sage: L=K^2 sage: R=L.span([[x,1/x]]) sage: R.basis()[0][0] x sage: R.basis()[0][0].parent() Fraction Field of Univariate Polynomial Ring in x over Rational Field sage: R=L.span([[x,x^2]]) sage: R.basis()[0][0].parent() Univariate Polynomial Ring in x over Rational Field
Since if we are going to look at the parent of one specific element, then we certainly want that it depends in the right way of the other elements.
A neater way to do this would be to actually store "coefficient_ring" in the parent, since I think that
parent.basis()[0][0].parent()
Is way to many steps removed from parent to actually directly acces from the code. But this is just nitpicking.
I'm now doctesting your patch.
comment:8 Changed 10 years ago by
I added a new patch, since we should also fix sparse modules (they suffer from the exact same bug). Running doctests now.
comment:9 Changed 10 years ago by
All tests pass so this again needs a review for the added sparse code.
comment:10 Changed 10 years ago by
- Description modified (diff)
comment:11 Changed 10 years ago by
- Description modified (diff)
- Reviewers set to Maarten Derickx, Julian Rueth
- Summary changed from make free_module_generic_pid also work for pid's other then integers to make free_module_generic_pid also work for pid's other than integers
comment:12 Changed 10 years ago by
- Status changed from needs_review to positive_review
the patches apply to 4.7.2.alpha2 and all doctests pass.
comment:13 Changed 10 years ago by
- Description modified (diff)
apply: 11751_free_module_generic_pid-fix.patch, trac_11751_free_module_generic_pid-review.patch and trac_11751_whitespace.patch
comment:14 Changed 10 years ago by
- Description modified (diff)
- Reviewers changed from Maarten Derickx, Julian Rueth to Julian Rueth, Maarten Derickx
comment:15 Changed 10 years ago by
- Merged in set to sage-4.7.2.alpha3
- Resolution set to fixed
- Status changed from positive_review to closed
comment:16 Changed 7 years ago by
I really don't like the way how this was fixed. First of all, why is this even a bug? If you need fractions, I think you should use the fraction field from the beginning.
Second, it causes the computation of a basis(!) every time that vector()
is called.
I am working on initialization of vectors in #17561 and I would actually like to revert this fix and make
sage: R.<x> = QQ[] sage: L = R^1 sage: L.span([(1/x,)])
a TypeError
again.
comment:17 Changed 7 years ago by
- Description modified (diff)
ok, I get it now. The issue is more complex than what the description shows.
comment:18 Changed 7 years ago by
Anyway, I'm fixing the basis()
issue by adding a new method basis_ring()
to free modules.
Changing the is into == is needed to make the example in the description work but in introduced some other strange bugs: