Opened 6 years ago

Closed 2 years ago

#14064 closed defect (fixed)

Matrix.subdivide goes haywire if indices are not in increasing order

Reported by: Bouillaguet Owned by: Bouillaguet
Priority: major Milestone: sage-7.3
Component: linear algebra Keywords: beginner, sd75
Cc: Merged in:
Authors: Fangan Dosso Reviewers: Édouard Rousseau, Turku Ozlum Celik
Report Upstream: N/A Work issues:
Branch: 10460b4 (Commits) Commit: 10460b458991481f175c9aec0ef8a9523872f303
Dependencies: Stopgaps:

Description (last modified by Bouillaguet)

sage: M = identity_matrix(5)
sage: M.subdivide([4,1], [3,2])
sage: M
[1 0|0|0 0]
[-----++-----]
[0 1|0|0 0]
[0 0|1|0 0]
[0 0|0|1 0]
[-----++-----]
[0 0|0|0 1]
sage: M.subdivision(1,1)
[]

This is broken, and there is an easy fix.

Change History (21)

comment:1 Changed 5 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:2 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:3 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:4 Changed 4 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:5 Changed 2 years ago by jakobkroeker

  • Keywords trivial added

comment:6 Changed 2 years ago by tscrim

  • Keywords trivial removed
  • Milestone changed from sage-6.4 to sage-7.3

It would be good for you to provide a fix!

comment:7 Changed 2 years ago by Bouillaguet

  • Keywords beginner sd75 added
  • Owner changed from jason, was to Bouillaguet

comment:8 Changed 2 years ago by Bouillaguet

  • Description modified (diff)

comment:9 Changed 2 years ago by fdosso48

  • Branch set to u/fdosso48/bug_in_subdivide

comment:10 Changed 2 years ago by fdosso48

  • Commit set to 60752486adc1cd7490a748f28f45ed2b43c309a8

The 'subdivide' method just needed its inputs sorted to work correctly.


New commits:

6075248Fixed a bug related to subdivide method

comment:11 Changed 2 years ago by fdosso48

  • Status changed from new to needs_review

comment:12 Changed 2 years ago by Bouillaguet

  • Status changed from needs_review to needs_work

The fix seems to work as expected. Could you please:

  • Add your real name in the "Author" Field of the ticket
  • Improve the documentation of the function (as discussed)
  • Add a doctest enforcing that the problem is fixed

After that, the tickets get a positive review (if the patchbot agrees).

Last edited 2 years ago by Bouillaguet (previous) (diff)

comment:13 Changed 2 years ago by fdosso48

  • Authors set to Fangan Dosso

comment:14 Changed 2 years ago by git

  • Commit changed from 60752486adc1cd7490a748f28f45ed2b43c309a8 to 10460b458991481f175c9aec0ef8a9523872f303

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

482ae02added doctest
10460b4Improves a little the 'subdivide' method documentation

comment:15 Changed 2 years ago by fdosso48

  • Status changed from needs_work to needs_review

comment:16 Changed 2 years ago by erousseau

Hi,

I went through the reviewer's checklist and everything looked fine.

Édouard

comment:17 follow-up: Changed 2 years ago by Bouillaguet

Then you should add you full name to the "Reviewers" field of the ticket, and mark it as "positive_review"

comment:18 Changed 2 years ago by erousseau

  • Reviewers set to Édouard Rousseau
  • Status changed from needs_review to positive_review

As I said just a little above, everything seemed fine.

Giving it a positive review.

Édouard

comment:19 in reply to: ↑ 17 Changed 2 years ago by erousseau

Replying to Bouillaguet:

Then you should add you full name to the "Reviewers" field of the ticket, and mark it as "positive_review"

OK, that's done.

comment:20 Changed 2 years ago by turkuozlum

  • Reviewers changed from Édouard Rousseau to Édouard Rousseau, Turku Ozlum Celik

I checked the ticket by following the reviewer's checklist. It seems that is OK. Positive review.

comment:21 Changed 2 years ago by vbraun

  • Branch changed from u/fdosso48/bug_in_subdivide to 10460b458991481f175c9aec0ef8a9523872f303
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.