Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#23867 closed defect (fixed)

Fix SciPy lil_matrix() with Sage Integers

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-8.1
Component: packages: standard Keywords:
Cc: zimmerma Merged in:
Authors: Jeroen Demeyer Reviewers: Travis Scrimshaw
Report Upstream: Reported upstream. Developers acknowledge bug. Work issues:
Branch: 854f588 (Commits) Commit:
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

Since the upgrade to SciPy 0.19.1, this is broken:

sage: from scipy.sparse import lil_matrix
sage: lil_matrix((100,100))
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-2-63750b5b39bc> in <module>()
----> 1 lil_matrix((Integer(100),Integer(100)))

/usr/local/src/sage-config/local/lib/python2.7/site-packages/scipy/sparse/lil.py in __init__(self, arg1, shape, dtype, copy)
    112                     self.data[i] = []
    113             else:
--> 114                 raise TypeError('unrecognized lil_matrix constructor usage')
    115         else:
    116             # assume A is dense

TypeError: unrecognized lil_matrix constructor usage

Upstream:

  1. https://github.com/scipy/scipy/pull/7864
  2. https://github.com/numpy/numpy/pull/9691

Change History (13)

comment:1 Changed 2 years ago by zimmerma

  • Cc zimmerma added

comment:2 Changed 2 years ago by jdemeyer

  • Branch set to u/jdemeyer/fix_scipy_lil_matrix___with_sage_integers

comment:3 Changed 2 years ago by jdemeyer

  • Commit set to 854f588f90b22fc1d1452a9e9d81b0ee8595b649
  • Status changed from new to needs_review

New commits:

854f588Fix SciPy lil_matrix() with Sage Integers

comment:4 Changed 2 years ago by jdemeyer

  • Report Upstream changed from N/A to Reported upstream. No feedback yet.

comment:5 follow-up: Changed 2 years ago by zimmerma

is that a temporary fix, waiting for the numpy/scipy issue to be fixed, and that will be removed afterwards?

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

Replying to zimmerma:

is that a temporary fix, waiting for the numpy/scipy issue to be fixed, and that will be removed afterwards?

Yes, exactly.

comment:7 Changed 2 years ago by jdemeyer

  • Report Upstream changed from Reported upstream. No feedback yet. to Reported upstream. Developers acknowledge bug.

comment:8 Changed 2 years ago by jdemeyer

  • Authors set to Jeroen Demeyer

comment:9 Changed 2 years ago by jdemeyer

  • Description modified (diff)

comment:10 Changed 2 years ago by tscrim

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

Upstream has essentially accepted the PR, and everything else LGTM.

comment:11 Changed 2 years ago by vbraun

  • Branch changed from u/jdemeyer/fix_scipy_lil_matrix___with_sage_integers to 854f588f90b22fc1d1452a9e9d81b0ee8595b649
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:12 follow-up: Changed 2 years ago by zimmerma

  • Commit 854f588f90b22fc1d1452a9e9d81b0ee8595b649 deleted

just a question: since this is a temporary fix, is there a way to ensure we don't forget to revert it when the issue is fixed upstream?

comment:13 in reply to: ↑ 12 Changed 2 years ago by jdemeyer

Replying to zimmerma:

just a question: since this is a temporary fix, is there a way to ensure we don't forget to revert it when the issue is fixed upstream?

If numpy accepts the Sage patch, we will notice that the Sage patch to numpy no longer applies when we upgrade numpy.

Note: See TracTickets for help on using tickets.