Opened 5 years ago

Closed 5 years ago

#18147 closed defect (fixed)

Rank of int matrix, documentation typo

Reported by: jmantysalo Owned by:
Priority: trivial Milestone: sage-6.7
Component: linear algebra Keywords:
Cc: vdelecroix Merged in:
Authors: Jori Mäntysalo Reviewers: Frédéric Chapoton, Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: fc49c4f (Commits) Commit: fc49c4f998eab90956699da5e523ce7f36c2c60a
Dependencies: Stopgaps:

Description (last modified by jmantysalo)

Output section of rank() contains input. Also Matrix.random(ZZ, 3).rank(algorithm='cat-says-meow') gives no error message.

Change History (13)

comment:1 Changed 5 years ago by jmantysalo

  • Branch set to u/jmantysalo/rank_of_int_matrix__documentation_typo

comment:2 Changed 5 years ago by jmantysalo

  • Commit set to 91cb99a1854dd5efcace52d1522d44de0c474e2e
  • Description modified (diff)
  • Status changed from new to needs_review

New commits:

​91cb99aFixed doc typo; added check for parameter 'algorithm'.

comment:3 Changed 5 years ago by vdelecroix

  • Cc vdelecroix added

I do not like this extra if. Could you rather change the code to: 

if algorithm == 'flint':
    ...
elif algorithm == 'modp':
    ...
elif algorithm == 'linbox':
    ...
else:
    raise ValueError

comment:4 Changed 5 years ago by jmantysalo

See the comment line 

# Algorithm is 'linbox' or detecting full rank didn't work . . .

This can be done, but I think that it would make code a little unclear.

comment:5 follow-up: Changed 5 years ago by jdemeyer

  • Status changed from needs_review to needs_work

What's with the changes in indendation?

comment:6 in reply to: ↑ 5 Changed 5 years ago by jmantysalo

Replying to jdemeyer:

What's with the changes in indendation?

The line had 11 spaces. I have thinked that Python should have 4n spaces. It's not at all the only line like that, see 

egrep '^ {4}* {1,3}[^ ]' src/sage/matrix/matrix_integer_dense.pyx

comment:7 follow-up: Changed 5 years ago by jmantysalo

  • Status changed from needs_work to needs_review

If my explanation of spaces is good enought, maybe this can be closed?

comment:8 in reply to: ↑ 7 Changed 5 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Replying to jmantysalo:

If my explanation of spaces is good enough

No, it's not good enough. I still don't understand why you are changing the indendation on some lines from correct to wrong (please look at the diff!)

comment:9 Changed 5 years ago by chapoton

  • Branch changed from u/jmantysalo/rank_of_int_matrix__documentation_typo to public/ticket/18147
  • Commit changed from 91cb99a1854dd5efcace52d1522d44de0c474e2e to fc49c4f998eab90956699da5e523ce7f36c2c60a

I have made a review commit. Thinks look good to me, but other reviewer should agree before this is set to positive review.

New commits:

​1c8490cMerge branch 'u/jmantysalo/rank_of_int_matrix__documentation_typo' into 6.6.rc3
​fc49c4ftrac #18147 review commit

comment:10 Changed 5 years ago by chapoton

  • Reviewers set to Frédéric Chapoton
  • Status changed from needs_work to needs_review

comment:11 Changed 5 years ago by jdemeyer

  • Reviewers changed from Frédéric Chapoton to Frédéric Chapoton, Jeroen Demeyer
  • Status changed from needs_review to positive_review

comment:12 Changed 5 years ago by chapoton

  • Milestone changed from sage-6.6 to sage-6.7

comment:13 Changed 5 years ago by vbraun

  • Branch changed from public/ticket/18147 to fc49c4f998eab90956699da5e523ce7f36c2c60a
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.