Opened 2 years ago

Closed 2 years ago

#25505 closed defect (fixed)

Clean up __cinit__ methods of matrices

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-8.3
Component: linear algebra Keywords:
Cc: tscrim Merged in:
Authors: Jeroen Demeyer Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: c8fa7bb (Commits) Commit: c8fa7bb27f4724a7263d76bec437ebcf7f87dec3
Dependencies: #25511 Stopgaps:

Description (last modified by jdemeyer)

  1. Generic stuff like processing the parent should be done by the base class (matrix0.Matrix in this case) instead of by each derived class individually. This means that we will require that the first argument of every matrix constructor is the parent. Because of #25511, this is currently true everywhere.
  1. In some cases, __cinit__ calls __init__ of the base class. That is really backwards and should never be needed!

This is a requirement for #23719.

Change History (11)

comment:1 Changed 2 years ago by jdemeyer

  • Authors set to Jeroen Demeyer

comment:2 Changed 2 years ago by jdemeyer

  • Branch set to u/jdemeyer/clean_up___cinit___methods_of_matrices

comment:3 Changed 2 years ago by tscrim

  • Cc tscrim added
  • Commit set to 90a0b033a61df3b81793a160e6772ad5df7d051e

New commits:

90a0b03Clean up __cinit__ methods of matrices

comment:4 Changed 2 years ago by jdemeyer

  • Description modified (diff)

comment:5 Changed 2 years ago by jdemeyer

  • Dependencies set to #25511
  • Description modified (diff)

comment:6 Changed 2 years ago by git

  • Commit changed from 90a0b033a61df3b81793a160e6772ad5df7d051e to c8fa7bb27f4724a7263d76bec437ebcf7f87dec3

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

4eae803Making matrices use the new _echelon_in_place method.
e2f0550Specify that _echelon_in_place shall return the pivots
3c4a06dEnable _mul_long for matrices
1eaed37More stuff in the meataxe interface, and a meataxe helper function
8a06c0fFix docstring formatting
13da208Clean up creating Matrix_gfpn_dense matrices
cb52ee3Mark one doctest optional
c8fa7bbClean up __cinit__ methods of matrices

comment:7 Changed 2 years ago by jdemeyer

  • Status changed from new to needs_review

comment:8 Changed 2 years ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

LGTM.

comment:9 follow-up: Changed 2 years ago by jdemeyer

Does this also imply a positive review of the dependency #25511?

comment:10 in reply to: ↑ 9 Changed 2 years ago by tscrim

Replying to jdemeyer:

Does this also imply a positive review of the dependency #25511?

Only in the sense that #25511 works as it is suppose to by the doctests and the code looks okay. Hmm...I guess I am saying that aren't I?

comment:11 Changed 2 years ago by vbraun

  • Branch changed from u/jdemeyer/clean_up___cinit___methods_of_matrices to c8fa7bb27f4724a7263d76bec437ebcf7f87dec3
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.