Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#23731 closed enhancement (fixed)

py3: future division in matrix2

Reported by: chapoton Owned by:
Priority: major Milestone: sage-8.1
Component: python3 Keywords: division
Cc: Merged in:
Authors: Frédéric Chapoton Reviewers: André Apitzsch, Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: fdf0a68 (Commits, GitHub, GitLab) Commit:
Dependencies: Stopgaps:

Status badges

Description

part of #15995

found using the "python -3" tests suggested there

Change History (11)

comment:1 Changed 5 years ago by chapoton

Branch: → u/chapoton/23731
Commit: → e65a63e5cc7fda873d9ed34ded4d8c72d3a666f5
Status: new → needs_review

New commits:

​e65a63epy3: future division in matrix2

comment:2 Changed 5 years ago by chapoton

Summary: py3: future division in matrix 2 → py3: future division in matrix2

comment:3 Changed 5 years ago by aapitzsch

Reviewers: → André Apitzsch
Status: needs_review → positive_review

LGTM.

comment:4 Changed 5 years ago by jdemeyer

Status: positive_review → needs_work

I disagree here: why hard-code QQ? Shouldn't we use self._parent._base instead? 

@@ -1371,7 +1372,7 @@ cdef class Matrix(Matrix1):
         # now compute the permanental minor of the complement matrix if needed
         if complement:
             a = [one]
-            c1 = 1
+            c1 = QQ.one()
             for k in range(1, mn + 1):
                 c1 = c1*(m-k+1)*(n-k+1)/k
                 c = c1

comment:5 Changed 5 years ago by chapoton

Because for this precise variable, we can stay in QQ. Otherwise, more complicated computations are done.

comment:6 Changed 5 years ago by git

Commit: e65a63e5cc7fda873d9ed34ded4d8c72d3a666f5 → fdf0a68c66108c7d370e5d23bff50d410e2f40a0

Branch pushed to git repo; I updated commit sha1. New commits:

​fdf0a68trac 23731 try exact division

comment:7 Changed 5 years ago by chapoton

So, what about the new commit ? The first makes sense (binomial coefficients) but i would not swear that the second one does make sense too.

New commits:

​fdf0a68trac 23731 try exact division

comment:8 Changed 5 years ago by chapoton

Status: needs_work → needs_review

comment:9 Changed 5 years ago by jdemeyer

Reviewers: André Apitzsch → André Apitzsch, Jeroen Demeyer
Status: needs_review → positive_review

comment:10 Changed 5 years ago by vbraun

Branch: u/chapoton/23731 → fdf0a68c66108c7d370e5d23bff50d410e2f40a0
Resolution: → fixed
Status: positive_review → closed

comment:11 Changed 5 years ago by embray

Commit: fdf0a68c66108c7d370e5d23bff50d410e2f40a0
Keywords: division added
Note: See TracTickets for help on using tickets.