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
Copy constructor #55
Conversation
self.sorted = coords.sorted | ||
self.shape = coords.shape | ||
return | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
flake8
is crapping out here. You can have a maximum of one continuous blank line inside of a function.
sparse/core.py
Outdated
@@ -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] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Some minor comments. |
Actually, it'd be nice to have a test for equality. Something like generating a random matrix and then |
Do we also want to ensure that the data and coords arrays are not the same?
…On Tue, Jan 2, 2018 at 3:23 AM, Hameer Abbasi ***@***.***> wrote:
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.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<https://github.com/mrocklin/sparse/pull/55#issuecomment-354747301>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AASszJxuWNBh7wzpqOYMGeCXHrch2Aifks5tGhHJgaJpZM4RPjj6>
.
|
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 |
OK, sounds good to me
…On Tue, Jan 2, 2018 at 9:23 AM, Hameer Abbasi ***@***.***> wrote:
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).
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<https://github.com/mrocklin/sparse/pull/55#issuecomment-354821260>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AASszHd90nAVvLeyfgRySctu0EbqZl7zks5tGmYtgaJpZM4RPjj6>
.
|
This is inspired by NumPy's behaviour, where people tend to do
numpy.array(a)
on any iterablea
, includingndarray
s, just to make sure it is anndarray
.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.