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

Inplace ops #146

Merged
merged 2 commits into from May 6, 2018

Conversation

Projects
None yet
4 participants
@hameerabbasi
Collaborator

hameerabbasi commented May 3, 2018

Closes #80

Leverages the existing __array_ufunc__ implementation without needing too much extra code.

@hameerabbasi hameerabbasi requested a review from mrocklin May 3, 2018

@codecov-io

This comment has been minimized.

codecov-io commented May 3, 2018

Codecov Report

Merging #146 into master will increase coverage by 0.64%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #146      +/-   ##
==========================================
+ Coverage   95.77%   96.42%   +0.64%     
==========================================
  Files          10       10              
  Lines        1183     1174       -9     
==========================================
- Hits         1133     1132       -1     
+ Misses         50       42       -8
Impacted Files Coverage Δ
sparse/coo/umath.py 96.9% <ø> (+0.47%) ⬆️
sparse/coo/core.py 94.63% <100%> (+0.92%) ⬆️
sparse/coo/indexing.py 100% <0%> (ø) ⬆️
sparse/coo/common.py 97.04% <0%> (+0.92%) ⬆️
sparse/dok.py 95.32% <0%> (+1.74%) ⬆️

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 444655c...92e3666. Read the comment docs.

@mrocklin

This comment has been minimized.

Collaborator

mrocklin commented May 3, 2018

This looks pretty slick :)

Might want to test something like the following as well:

np.sin(x, out=x)
else:
return NotImplemented
if out is not None:
out = out[0]

This comment has been minimized.

@mrocklin

mrocklin May 3, 2018

Collaborator

I've seen people do things like the following to ensure length

(out,) = out
@dhirschfeld

This comment has been minimized.

dhirschfeld commented May 3, 2018

Might necessitate a docs update?

However some operations are not supported, like inplace operations,

@hameerabbasi hameerabbasi force-pushed the hameerabbasi:inplace-ops branch from 09edda0 to f753894 May 3, 2018

@hameerabbasi

This comment has been minimized.

Collaborator

hameerabbasi commented May 3, 2018

Docs updated, warning added to docs that "in-place ops aren't really in-place".

Supported out kwarg for all ops now, including round and COO.sum, etc.

@hameerabbasi

This comment has been minimized.

Collaborator

hameerabbasi commented May 3, 2018

cc @mrocklin I think this is ready for a final review. :-)

If ready, please do a rebase+merge, I've kept the changes separate.

@hameerabbasi hameerabbasi force-pushed the hameerabbasi:inplace-ops branch 4 times, most recently from 847eea8 to 1965051 May 3, 2018

@hameerabbasi hameerabbasi force-pushed the hameerabbasi:inplace-ops branch from 1965051 to 92e3666 May 4, 2018

@hameerabbasi

This comment has been minimized.

Collaborator

hameerabbasi commented May 4, 2018

Removed some unnecessary code for astype -- It doesn't have an out argument. It was what was causing coverage to fail.

@mrocklin mrocklin merged commit 3afd0e0 into pydata:master May 6, 2018

4 checks passed

ci/circleci: build_27 Your tests passed on CircleCI!
Details
ci/circleci: build_36 Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 95.77%)
Details
codecov/project 96.42% (+0.64%) compared to 444655c
Details
@mrocklin

This comment has been minimized.

Collaborator

mrocklin commented May 6, 2018

Merging. Sorry for the delay in review. Thanks @hameerabbasi

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