New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add order parameter to `COO.reshape` #193

Merged
merged 2 commits into from Sep 19, 2018

Conversation

Projects
None yet
2 participants
@jcrist
Collaborator

jcrist commented Sep 19, 2018

Adds support for np.reshape to dispatch to COO.reshape properly.

Add order parameter to `COO.reshape`
Adds support for `np.reshape` to dispatch to `COO.reshape` properly.
@codecov

This comment has been minimized.

codecov bot commented Sep 19, 2018

Codecov Report

Merging #193 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #193      +/-   ##
==========================================
+ Coverage   97.52%   97.52%   +<.01%     
==========================================
  Files          11       11              
  Lines        1416     1417       +1     
==========================================
+ Hits         1381     1382       +1     
  Misses         35       35
Impacted Files Coverage Δ
sparse/coo/core.py 97.09% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f21b027...3daf0a6. Read the comment docs.

@hameerabbasi

One minor change.

# order parameter not actually supported
with pytest.raises(NotImplementedError):
np.reshape(s, shape, order='F')

This comment has been minimized.

@hameerabbasi

hameerabbasi Sep 19, 2018

Collaborator

We might want to remove these two lines from the test, if we add this functionality later it'll crap out.

This comment has been minimized.

@hameerabbasi

hameerabbasi Sep 19, 2018

Collaborator

An exception from coverage is already in place.

This comment has been minimized.

@jcrist

jcrist Sep 19, 2018

Collaborator

Done.

I personally prefer to check these errors - in the past I've seen code that was supposed to raise a DeprecationWarning or NotImplementedError fail to actually raise due to a bug. I don't think of the tests as the-one-uneditable-source-of-truth, I think someone adding this functionality wouldn't be surprised to see a test fail due to a NotImplementedError not being raised anymore, and be fine changing the test.

Anyway, happy to cede to you here. I believe this should be good to go.

This comment has been minimized.

@hameerabbasi

hameerabbasi Sep 19, 2018

Collaborator

Fair enough. 👍

@hameerabbasi hameerabbasi merged commit 8a29cdf into pydata:master Sep 19, 2018

5 checks passed

LGTM analysis: Python No alert changes
Details
ci/circleci Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 97.52%)
Details
codecov/project 97.52% (+<.01%) compared to f21b027
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jcrist jcrist deleted the jcrist:reshape-func branch Sep 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment