Opened 12 years ago

Closed 11 years ago

#1834 closed defect (fixed)

[with revised patch, with positive review] General linear group over ZZ hangs in __call__

Reported by: kohel Owned by: AlexGhitza
Priority: major Milestone: sage-3.1.2
Component: linear algebra Keywords:
Cc: AlexGhitza Merged in:
Authors: Reviewers:
Report Upstream: Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

sage: G = GL(3,GF(101)) sage: G(1,0,1],[0,1,0],[0,0,1?)

[1 0 1] [0 1 0] [0 0 1]

works fine, but

sage: G = GL(3,ZZ) sage: G(1,0,1],[0,1,0],[0,0,1?)

This should not try to find a solution to the word problem.

Attachments (1)

1834-gl_z_call.patch (4.9 KB) - added by AlexGhitza 11 years ago.

Download all attachments as: .zip

Change History (11)

comment:1 Changed 12 years ago by mabshoff

  • Milestone set to sage-2.10.1

comment:2 Changed 11 years ago by AlexGhitza

  • Cc AlexGhitza added

comment:3 Changed 11 years ago by AlexGhitza

  • Summary changed from General linear group over ZZ hangs in __call__ to [with patch, needs review] General linear group over ZZ hangs in __call__

This has been around for a while, and it's been bugging me. I fixed it by writing methods __call__ and __contains__ for the class !GeneralLinearGroup_generic, so that the GAP ones (which hang over ZZ) don't get used. A pleasant side effect is that things are now faster for the cases that were working before (i.e. over finite fields):

before:

sage: G = GL(5, GF(next_prime(6*10^4)))                                    
sage: m = [[1,0,1,0,2],[0,1,0,1,1],[0,0,1,0,0],[0,0,0,1,1],[0,0,0,0,1]]                              
sage: timeit("G(m)")                                                                                     
25 loops, best of 3: 9.56 ms per loop

after:

sage: G = GL(5, GF(next_prime(6*10^4)))                                    
sage: m = [[1,0,1,0,2], [0,1,0,1,1], [0,0,1,0,0], [0,0,0,1,1], [0,0,0,0,1]]                              
sage: timeit("G(m)")                                                                                     
625 loops, best of 3: 459 µs per loop

The same issue comes up for all the matrix groups. For the moment, I have only dealt with the really easy cases of GL and SL. If this gets approved and merged, I will open a new ticket for the other classes of groups and deal with them in a similar way.

comment:4 Changed 11 years ago by AlexGhitza

  • Owner changed from was to AlexGhitza

comment:5 Changed 11 years ago by AlexGhitza

  • Status changed from new to assigned

comment:6 Changed 11 years ago by cremona

  • Component changed from algebraic geometry to linear algebra
  • Summary changed from [with patch, needs review] General linear group over ZZ hangs in __call__ to [with patch, with review, work needed] General linear group over ZZ hangs in __call__

Basically ok, but I would make the following changes to both cases (GL and SL):

Use a try: except: block to catch when coercion in the matrix space fails, with the error message "Cannot coerce ... to a matrix". Then catch the non-invertible matrices in the GL case with the error message "... is not an invertible matrix", and similarly in the SL case with "... does not have determinant 1".

I think this alternative error handling will produce better output.

PS As this is not "algebraic geometry" I changed the Component to Linear Algebra

Changed 11 years ago by AlexGhitza

comment:7 Changed 11 years ago by AlexGhitza

Good idea. I have made the changes and replaced the patch with a new one.

comment:8 Changed 11 years ago by AlexGhitza

  • Summary changed from [with patch, with review, work needed] General linear group over ZZ hangs in __call__ to [with revised patch, needs review] General linear group over ZZ hangs in __call__

comment:9 Changed 11 years ago by cremona

  • Summary changed from [with revised patch, needs review] General linear group over ZZ hangs in __call__ to [with revised patch, with positive review] General linear group over ZZ hangs in __call__

Excellent. These are much more helpful messages.

The new patch applies ok to 3.1.2.alpha3 (there was a little fuzz:

applying /home/john/1834-gl_z_call.patch
patching file sage/groups/matrix_gps/matrix_group.py
Hunk #1 succeeded at 13 with fuzz 1 (offset 3 lines).

but nothing serious). All doctests in sage.groups.matrix_groups pass. Publish!

comment:10 Changed 11 years ago by mabshoff

  • Resolution set to fixed
  • Status changed from assigned to closed

Merged in Sage 3.1.2.alpha4

Note: See TracTickets for help on using tickets.