Skip to content
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

Copy constructor #55

Merged
merged 2 commits into from Jan 2, 2018
Merged

Copy constructor #55

merged 2 commits into from Jan 2, 2018

Conversation

@nils-werner
Copy link
Contributor

@nils-werner nils-werner commented Dec 30, 2017

This is inspired by NumPy's behaviour, where people tend to do numpy.array(a) on any iterable a, including ndarrays, just to make sure it is an ndarray.

Do you think something like this would make sense? Would save us having to do type checks in places where we just want to make sure it is a COO instance.

self.sorted = coords.sorted
self.shape = coords.shape
return

This comment has been minimized.

@hameerabbasi

hameerabbasi Dec 30, 2017
Collaborator

flake8 is crapping out here. You can have a maximum of one continuous blank line inside of a function.

@@ -1366,7 +1375,7 @@ def concatenate(arrays, axis=0):

def stack(arrays, axis=0):
assert len(set(x.shape for x in arrays)) == 1
arrays = [x if type(x) is COO else COO(x) for x in arrays]
arrays = [COO(x) for x in arrays]

This comment has been minimized.

@hameerabbasi

hameerabbasi Dec 30, 2017
Collaborator

I would leave these as-is, or maybe change them to arrays = [x if isinstance(x, COO) else COO(x) for x in arrays]. Generally, we want to avoid making copies where possible.

I realize what you are doing is a shallow copy, not a deep one, but in the future, if we allow manipulation of arrays (if we make the COO type mutable), we might need to change this to a deep copy, and that might be expensive.

@hameerabbasi
Copy link
Collaborator

@hameerabbasi hameerabbasi commented Dec 30, 2017

Some minor comments.

@hameerabbasi
Copy link
Collaborator

@hameerabbasi hameerabbasi commented Jan 2, 2018

Actually, it'd be nice to have a test for equality. Something like generating a random matrix and then COO(x) , asserting they're not the same object, and an assert_eq.

@mrocklin
Copy link
Collaborator

@mrocklin mrocklin commented Jan 2, 2018

@hameerabbasi
Copy link
Collaborator

@hameerabbasi hameerabbasi commented Jan 2, 2018

I don't know the convention in Python here. I know Numpy makes a deep copy but we might get away with a shallow one since we're not mutating the original data and coords in any way. I'm good with this until it raises issues (which I suspect won't be for a long time since COO is immutable).

@mrocklin
Copy link
Collaborator

@mrocklin mrocklin commented Jan 2, 2018

@hameerabbasi hameerabbasi merged commit 9b6caa9 into pydata:master Jan 2, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants