Opened 10 years ago

Closed 10 years ago

Last modified 6 years ago

#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:

Status badges

Description (last modified by jdemeyer)

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. 11751_free_module_generic_pid-fix.patch
  2. trac_11751_free_module_generic_pid-review.patch
  3. trac_11751_whitespace.patch

to the Sage library.

Attachments (3)

11751_free_module_generic_pid-fix.patch (3.0 KB) - added by mderickx 10 years ago.
trac_11751_free_module_generic_pid-review.patch (5.9 KB) - added by mderickx 10 years ago.
fixes the failing doctests
trac_11751_whitespace.patch (4.3 KB) - added by jdemeyer 10 years ago.
small review patch to fix some whitespace and doc issues

Download all attachments as: .zip

Change History (21)

Changed 10 years ago by mderickx

comment:1 Changed 10 years ago by mderickx

  • Description modified (diff)

comment:2 Changed 10 years ago by mderickx

  • Description modified (diff)

comment:3 Changed 10 years ago by mderickx

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

comment:4 Changed 10 years ago by mderickx

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

comment:5 Changed 10 years ago by saraedum

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 saraedum

  • Status changed from new to needs_review

comment:7 Changed 10 years ago by mderickx

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.

Changed 10 years ago by mderickx

fixes the failing doctests

comment:8 Changed 10 years ago by mderickx

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 mderickx

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

comment:10 Changed 10 years ago by mderickx

  • Description modified (diff)

comment:11 Changed 10 years ago by saraedum

  • Authors set to Maarten Derickx, Julian Rueth
  • 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 saraedum

  • 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 mderickx

  • 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 leif

  • Description modified (diff)
  • Reviewers changed from Maarten Derickx, Julian Rueth to Julian Rueth, Maarten Derickx

comment:15 Changed 10 years ago by leif

  • Merged in set to sage-4.7.2.alpha3
  • Resolution set to fixed
  • Status changed from positive_review to closed

Changed 10 years ago by jdemeyer

small review patch to fix some whitespace and doc issues

comment:16 Changed 6 years ago by jdemeyer

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 6 years ago by jdemeyer

  • Description modified (diff)

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

comment:18 Changed 6 years ago by jdemeyer

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

Note: See TracTickets for help on using tickets.