Opened 4 years ago

Closed 4 years ago

#23068 closed enhancement (fixed)

py3 more absolute imports in pyx files

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

Status badges

Description

part of #22808

Change History (11)

comment:1 Changed 4 years ago by chapoton

  • Branch set to u/chapoton/23068
  • Commit set to 030b0b596d1e146858c78c3eb9b6b138823911ae

New commits:

030b0b5more absolute imports in pyx files

comment:2 Changed 4 years ago by git

  • Commit changed from 030b0b596d1e146858c78c3eb9b6b138823911ae to b7eedc59e8eebaa135b27dd8ed9a78b9ccc24d9e

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

b7eedc5trac 23068 typo

comment:3 Changed 4 years ago by git

  • Commit changed from b7eedc59e8eebaa135b27dd8ed9a78b9ccc24d9e to a12d70010393a07196b4b451be1ccc153d33e1b2

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

a12d700more absolute imports in pyx files

comment:4 Changed 4 years ago by git

  • Commit changed from a12d70010393a07196b4b451be1ccc153d33e1b2 to 59237752defce7bffd861e80591da3246ec32d09

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

5923775trac 23068 fixing imports in matrix2

comment:5 Changed 4 years ago by chapoton

  • Cc jdemeyer added
  • Status changed from new to needs_review

bot is essentially green, please review

comment:6 Changed 4 years ago by jdemeyer

  • Status changed from needs_review to needs_work
  1. Why did you change
    if self._ncols == 0
    

to

if not self._ncols

I find self._ncols == 0 clearer to understand: it checks for a matrix with zero colomns.

  1. It is wrong to write sage.matrix.matrix_space.MatrixSpace without importing it. It might work by accident, but it's not good. Either you explicitly import sage.matrix.matrix_space or you do something like from sage.matrix.matrix_space import MatrixSpace and then use MatrixSpace.
  1. Same comment for sage.matrix.constructor.
  1. Same comment for sage.rings.power_series_ring in src/sage/rings/power_series_ring_element.pyx

comment:7 Changed 4 years ago by chapoton

About 1, ok

About 2,3,4:

I found this solution to break circular imports. If you think you can do better, please do. This ticket is very low on my priority list. If you feel like helping towards python3, I would appreciate any contribution to the discussion on why building with SAGE_PYTHON3 currently fails:

https://groups.google.com/forum/#!topic/sage-devel/FqR-37Fu3tY

comment:8 Changed 4 years ago by git

  • Commit changed from 59237752defce7bffd861e80591da3246ec32d09 to 7113b26fdcc440dbf4c2d6c94c0a08d40d963e42

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

ab723dcMerge branch 'u/chapoton/23068' in 8.0.b8
7113b26trac 23068 better imports

comment:9 Changed 4 years ago by chapoton

  • Status changed from needs_work to needs_review

should be better now

comment:10 Changed 4 years ago by aapitzsch

  • Reviewers set to Jeroen Demeyer, André Apitzsch
  • Status changed from needs_review to positive_review

LGTM.

comment:11 Changed 4 years ago by vbraun

  • Branch changed from u/chapoton/23068 to 7113b26fdcc440dbf4c2d6c94c0a08d40d963e42
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.