Drop support for Numpy 1.12. #84

hameerabbasi · 2018-01-19T00:41:51Z

Drop Numpy 1.12 support
Implement all operators via NDArrayOperatorsMixin.
Support for scipy.sparse.spmatrix operations from the left (mostly).
Raise TypeError for __array__.

hameerabbasi · 2018-01-19T01:55:15Z

Closes #81 as wontfix.

Edit: #81 will now be closed as fixed.

hameerabbasi · 2018-01-19T01:56:29Z

In addition, closes #78 as wontfix.

hameerabbasi · 2018-01-19T09:16:34Z

I've updated this PR with the excellent suggestions from @njsmith.

@nils-werner @mrocklin Nice if you gave it a look.

nils-werner · 2018-01-19T12:50:05Z

This is a radical change and needs a proper code review, but it would be a very sleek solution, if no issues pop up.

Also, it should warrant a major version bump, as it is pretty backwards-incompatible :-)

mrocklin · 2018-01-19T14:10:49Z

I agree with @nils-werner that this is sleek.

The dependence on 1.13 is interesting. I'm fine going forward with this, but we might want to release before we merge, just so we can point people not yet on latest numpy to that version.

Also, it should warrant a major version bump, as it is pretty backwards-incompatible :-)

We haven't released in a good long while. I'll add this (and instructions) to my TODO list if no one has objections.

hameerabbasi · 2018-01-19T15:06:48Z

Here's what I propose (in no particular order):

We tidy up Deal with np.array(sparsearr) densification #72 and merge it to a 0.2 (or whatever) branch.
We discuss and make this one proper and merge this to master.

Even if we do the latter first, we can always make a 0.2 branch from the previous commit. Git is awesome that way. :)

mrocklin · 2018-01-23T14:35:43Z

Any objection to merging #72 to master, releasing, and then merging this one on top of it? I have a slight desire to keep releases in the mainline history. Happy either way though. I'd also be happy if you wanted to just skip directly to dropping numpy 1.13.

hameerabbasi · 2018-01-23T14:56:12Z

Releases will diverge from mainline history if we make backwards incompatible changes (as we are doing here) and then have to fix bugs in old releases. This is the reason why I would prefer any backwards-incompatible changes happen on master and the older version be in a separate branch.

This applies particularly to this PR and #72 because we're effectively undoing #72 by merging this, and it makes more sense to me intuitively that they should be in separate branches.

However, I would still prefer to release with #72 merged for the reason that not only are we dropping support for Numpy 1.12 (I have no problem with that part), but we're also dropping support for inter-compatibility with scipy.sparse and requiring that all objects be explicitly converted to COO before any operations.

Edit:

skip directly to dropping numpy 1.13.

I assume that's a typo. We're dropping Numpy 1.12, not 1.13. I'm not in favor of dropping old versions as soon as new ones come out. ;-)

hameerabbasi · 2018-01-23T15:20:30Z

I personally am fine with both, releasing with #72 or this PR. I don't know if if you have reservations, though. Because dropping Numpy 1.12 support only requires users to update Numpy, dropping scipy.sparse inter-op requires... well... Rewriting code.

mrocklin · 2018-01-25T00:10:45Z

This applies particularly to this PR and #72 because we're effectively undoing #72 by merging this, and it makes more sense to me intuitively that they should be in separate branches.

I'm totally fine having a commit in mainline history than we undo in a subsequent commit.

However, I would still prefer to release with #72 merged for the reason that not only are we dropping support for Numpy 1.12 (I have no problem with that part), but we're also dropping support for inter-compatibility with scipy.sparse and requiring that all objects be explicitly converted to COO before any operations.

Hrm, I hadn't realized the change with support for scipy.sparse. Is this critical to achieve the nice refactor that you have here? Was it desired?

mrocklin · 2018-01-25T00:11:21Z

I assume that's a typo. We're dropping Numpy 1.12, not 1.13. I'm not in favor of dropping old versions as soon as new ones come out. ;-)

Not a typo, just me being distracted and forgetting which versions of Numpy are out :)

hameerabbasi · 2018-01-25T12:57:38Z

I'm totally fine having a commit in mainline history than we undo in a subsequent commit.

In that case, we should be good.

Hrm, I hadn't realized the change with support for scipy.sparse. Is this critical to achieve the nice refactor that you have here? Was it desired?

I added it back, I realized it was too drastic. In any case, supporting scipy.sparse operator COO seems to be a hit-and-miss. add and sub works fine, but mul tries to densify and lt/gt/ne all raise TypeErrors. We should document, at the very least, that it is recommended to convert all arrays to COO before starting.

See tests for details.

Edit: This happens regardless of this change.

mrocklin · 2018-01-25T13:00:26Z

Ah, mul for scipy.sparse will be matrix multiplication due to the np.matrix interface I think

…

On Thu, Jan 25, 2018 at 7:57 AM, Hameer Abbasi ***@***.***> wrote: I'm totally fine having a commit in mainline history than we undo in a subsequent commit. In that case, we should be good. Hrm, I hadn't realized the change with support for scipy.sparse. Is this critical to achieve the nice refactor that you have here? Was it desired? I added it back, I realized it was too drastic. In any case, supporting scipy.sparse operator COO seems to be a hit-and-miss. add and sub works fine, but mul tries to densify and lt/gt/ne all raise TypeErrors. We should document, at the very least, that it is recommended to convert all arrays to COO before starting. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <https://github.com/mrocklin/sparse/pull/84#issuecomment-360459263>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AASszDidgI-ylW4BEDYyNm3s-53xnLMSks5tOHpCgaJpZM4Rj0qa> .

mrocklin · 2018-01-25T13:00:38Z

I would not be surprised to see that behave oddly

…

On Thu, Jan 25, 2018 at 8:00 AM, Matthew Rocklin ***@***.***> wrote: Ah, mul for scipy.sparse will be matrix multiplication due to the np.matrix interface I think On Thu, Jan 25, 2018 at 7:57 AM, Hameer Abbasi ***@***.***> wrote: > I'm totally fine having a commit in mainline history than we undo in a > subsequent commit. > > In that case, we should be good. > > Hrm, I hadn't realized the change with support for scipy.sparse. Is this > critical to achieve the nice refactor that you have here? Was it desired? > > I added it back, I realized it was too drastic. In any case, supporting scipy.sparse > operator COO seems to be a hit-and-miss. add and sub works fine, but mul > tries to densify and lt/gt/ne all raise TypeErrors. We should document, > at the very least, that it is recommended to convert all arrays to COO > before starting. > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <https://github.com/mrocklin/sparse/pull/84#issuecomment-360459263>, or mute > the thread > <https://github.com/notifications/unsubscribe-auth/AASszDidgI-ylW4BEDYyNm3s-53xnLMSks5tOHpCgaJpZM4Rj0qa> > . >

hameerabbasi · 2018-01-25T16:41:19Z

This seems okay to merge from my side.

hameerabbasi · 2018-01-26T20:34:26Z

I removed calls of the form COO.ufunc because np.ndarray doesn't have them either. np.ufunc(COO, ...) still works because of the all-powerful __array_ufunc__.

hameerabbasi · 2018-01-26T21:05:18Z

Also, it should warrant a major version bump, as it is pretty backwards-incompatible :-)

We're only dropping support for Numpy 1.12. Scipy.sparse inter-op has been added back. This slims down our code quite a bit.

hameerabbasi · 2018-01-27T09:21:06Z

I think this is ready for a code review + merge. :-)

* Add tests for scipy spmatrices from the left. * Fix failing tests. * Update docs. * Add back pure Python abs support. * Drop Scipy op support * Add Scipy sparse support back. * Update docs. * Remove most .ufunc calls since Numpy doesn't have them. * Update docs.

hameerabbasi mentioned this pull request Jan 19, 2018

Deal with np.array(sparsearr) densification #72

Closed

hameerabbasi changed the title ~~Nd array operators mixin~~ Drop support for scipy.sparse ops and Numpy 1.12. Jan 19, 2018

hameerabbasi force-pushed the NDArrayOperatorsMixin branch from e593fa9 to 141a025 Compare January 23, 2018 15:15

hameerabbasi changed the title ~~Drop support for scipy.sparse ops and Numpy 1.12.~~ Drop support for Numpy 1.12. Jan 25, 2018

hameerabbasi mentioned this pull request Jan 25, 2018

Implement __array__ protocol #87

Merged

hameerabbasi added 7 commits January 25, 2018 16:13

Add tests for scipy spmatrices from the left.

00423dc

Fix failing tests.

08e559b

Update docs.

e36bc8c

Add back pure Python abs support.

e38f548

Drop Scipy op support

3951ac5

Add Scipy sparse support back.

4809648

Update docs.

5ccdbaa

hameerabbasi force-pushed the NDArrayOperatorsMixin branch from a68fde7 to 5ccdbaa Compare January 25, 2018 15:41

Remove most .ufunc calls since Numpy doesn't have them.

3a92642

Update docs.

316022d

mrocklin merged commit 731c06a into pydata:master Jan 27, 2018

hameerabbasi deleted the NDArrayOperatorsMixin branch January 27, 2018 14:31

This was referenced Jan 28, 2018

Test NumPy 1.12 and latest in both Travis and tox #79

Closed

Operations of the form scipy.sparse.spmatrix operator COO return a dense result #81

Closed

NumPy 1.12.0 has failing tests #78

Closed

hameerabbasi mentioned this pull request Feb 10, 2018

Support Everything that XArray Expects #1

Open

16 tasks

Uh oh!

Drop support for Numpy 1.12. #84

Drop support for Numpy 1.12. #84

Uh oh!

Conversation

hameerabbasi commented Jan 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hameerabbasi commented Jan 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hameerabbasi commented Jan 19, 2018

Uh oh!

hameerabbasi commented Jan 19, 2018

Uh oh!

nils-werner commented Jan 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrocklin commented Jan 19, 2018

Uh oh!

hameerabbasi commented Jan 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrocklin commented Jan 23, 2018

Uh oh!

hameerabbasi commented Jan 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hameerabbasi commented Jan 23, 2018

Uh oh!

mrocklin commented Jan 25, 2018

Uh oh!

mrocklin commented Jan 25, 2018

Uh oh!

hameerabbasi commented Jan 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrocklin commented Jan 25, 2018 via email

Uh oh!

mrocklin commented Jan 25, 2018 via email

Uh oh!

hameerabbasi commented Jan 25, 2018

Uh oh!

hameerabbasi commented Jan 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hameerabbasi commented Jan 26, 2018

Uh oh!

hameerabbasi commented Jan 27, 2018

Uh oh!

Uh oh!

hameerabbasi commented Jan 19, 2018 •

edited

Loading

hameerabbasi commented Jan 19, 2018 •

edited

Loading

nils-werner commented Jan 19, 2018 •

edited

Loading

hameerabbasi commented Jan 19, 2018 •

edited

Loading

hameerabbasi commented Jan 23, 2018 •

edited

Loading

hameerabbasi commented Jan 25, 2018 •

edited

Loading

hameerabbasi commented Jan 26, 2018 •

edited

Loading