make free_module_generic_pid also work for pid's other than integers

[koffie]

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

1. attachment: 11751_free_module_generic_pid-fix.patch
2. attachment: trac_11751_free_module_generic_pid-review.patch
3. attachment: trac_11751_whitespace.patch
   to the Sage library.

Component: linear algebra

Author: Maarten Derickx, Julian Rueth

Reviewer: Julian Rueth, Maarten Derickx

Merged: sage-4.7.2.alpha3

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

[koffie]

comment:1

Attachment: 11751_free_module_generic_pid-fix.patch.gz

[koffie]

comment:3

Changing the is into == is needed to make the example in the description work but in introduced some other strange bugs:

sage -t  devel/sage-main/sage/modules/quotient_module.py # 1 doctests failed
sage -t  devel/sage-main/sage/modules/fg_pid/fgp_element.py # 3 doctests failed
sage -t  devel/sage-main/sage/modules/free_module_element.pyx # 3 doctests failed

[koffie]

comment:4

A full output of the errors is shown in http://pastebin.com/4B4xmRDZ

[saraedum]

comment:5

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.

[koffie]

comment:7

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.

[koffie]

Attachment: trac_11751_free_module_generic_pid-review.patch.gz

fixes the failing doctests

[koffie]

comment:8

I added a new patch, since we should also fix sparse modules (they suffer from the exact same bug). Running doctests now.

[koffie]

comment:9

All tests pass so this again needs a review for the added sparse code.

[saraedum]

Author: Maarten Derickx, Julian Rueth

[saraedum]

Reviewer: Maarten Derickx, Julian Rueth

[saraedum]

comment:12

the patches apply to 4.7.2.alpha2 and all doctests pass.

[koffie]

comment:13

apply: 11751_free_module_generic_pid-fix.patch, trac_11751_free_module_generic_pid-review.patch and trac_11751_whitespace.patch

[nexttime]

Changed reviewer from Maarten Derickx, Julian Rueth to Julian Rueth, Maarten Derickx

[nexttime]

Merged: sage-4.7.2.alpha3

[jdemeyer]

small review patch to fix some whitespace and doc issues

[jdemeyer]

comment:16

Attachment: trac_11751_whitespace.patch.gz

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.

[jdemeyer]

comment:17

ok, I get it now. The issue is more complex than what the description shows.

[jdemeyer]

comment:18

Anyway, I'm fixing the basis() issue by adding a new method basis_ring() to free modules.