Refactor and clean format conversions. #152

hameerabbasi · 2018-05-09T11:14:50Z

Closes #151

hameerabbasi · 2018-05-09T11:15:17Z

cc @ahwillia I encourage you to take a look, if you can.

codecov-io · 2018-05-09T11:16:16Z

Codecov Report

Merging #152 into master will increase coverage by 0.47%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #152      +/-   ##
==========================================
+ Coverage   96.42%   96.89%   +0.47%     
==========================================
  Files          10       10              
  Lines        1174     1191      +17     
==========================================
+ Hits         1132     1154      +22     
+ Misses         42       37       -5

Impacted Files	Coverage Δ
sparse/coo/__init__.py	`100% <100%> (ø)`	⬆️
sparse/coo/core.py	`96.36% <100%> (+1.72%)`	⬆️
sparse/dok.py	`95.57% <100%> (+0.24%)`	⬆️
sparse/utils.py	`97.05% <100%> (-0.09%)`	⬇️
sparse/sparse_array.py	`92% <100%> (+0.33%)`	⬆️

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 3afd0e0...5e0f36b. Read the comment docs.

hameerabbasi · 2018-05-12T18:24:31Z

cc @mrocklin Is it possible to have a quicker review on this? It's blocking my progress on fill-values.

mrocklin · 2018-05-12T18:32:16Z

My apologies for the delay. I'm at a conference this week and may not have a ton of time. I'll put this high-ish on my todo list, but it's a slow moving list at the moment. My apologies for being a bottleneck.

…

On Sat, May 12, 2018 at 2:24 PM, Hameer Abbasi ***@***.***> wrote: cc @mrocklin <https://github.com/mrocklin> Is it possible to have a quicker review on this? It's blocking my progress on fill-values. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#152 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AASszK6nkjHCZ8HCLrnmnvX68zMJLKmLks5txyjfgaJpZM4T4I63> .

hameerabbasi · 2018-05-12T18:33:13Z

Ah, take your time, in that case.

mrocklin · 2018-05-12T18:41:04Z

@jcrist *might* be interested in starting reviewing some code in this project.

…

On Sat, May 12, 2018 at 2:33 PM, Hameer Abbasi ***@***.***> wrote: Ah, take your time, in that case. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#152 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AASszLbAHuRYqKLDigyXfXbZxtiv184Rks5txyr2gaJpZM4T4I63> .

hameerabbasi · 2018-05-12T18:43:05Z

It's always awesome to have more people involved! Developers, users, reviewers, or any other kind of role. 😃

ahwillia · 2018-05-13T03:46:55Z

These changes generally look good to me. Now that broadcasting works in the COO constructor it seems quite intuitive to me. Does this supercede #150 ? It doesn't seem like there are any conflicts, but maybe sparse.full(...) is now unnecessary / redundant? I'm happy to close that PR if so.

If we were to keep sparse.full(...) would it make sense to add a format keyword? Right now the function is only able to produce a COO array.

hameerabbasi · 2018-05-13T05:10:50Z

I hadn't added data broadcasting yet when you last commented, but I just did (along with a docs update). I'll leave the decision of whether or not to add sparse.full up to you. IMHO, it's unnecessary, and it doesn't match up exactly with np.full. In this library, we try to make things match exactly where possible.

In that light, sparse.full should be an array with all-same values. This will be possible (or rather feasible) after #143 is implemented.

ahwillia · 2018-05-14T01:44:20Z

In that light, sparse.full should be an array with all-same values.

I agree with this 👍

mrocklin

This seems good to me. A few small comments. My apologies for the late review.

mrocklin · 2018-05-14T12:07:36Z

sparse/coo/core.py

+            x = list(x.items())
+
+        if len(x) != 2 and not all(len(item) == 2 for item in x):
+            raise ValueError('Invalid iterable to convert to COO.')


Is it possible that they user will give an Iterator rather than an Iterable? If so then this check will consume our data. We might want to defensively convert to a tuple or list.

I'm not too familiar with the differences between the two. Will this check break or is it just a performance consideration?

Iterators are ephemeral and get consumed on first use. They are common, for example, when reading a sequence of data from a file. Here is an example that creates an iterator (seq) from an iterable (L) does a check, and then discovers that some values have been removed:

In [1]: L = [1, 2, 3] In [2]: seq = iter(L) In [3]: if all(x > 100 for x in seq): ...: print("hello") ...: In [4]: list(seq) Out[4]: [2, 3] In [5]: from collections import Iterator, Iterable In [7]: isinstance(seq, Iterator) Out[7]: True In [8]: isinstance(L, Iterable) Out[8]: True In [9]: L Out[9]: [1, 2, 3]

Ah, I understand now. I don't think making this a supported input is worth the extra effort.

I suspect that users will use iterators here whether or not we support it. Currently we silently fail. We should either fail loudly

if isinstance(x, Iterator): raise TypeError("...")

Or we should coerce to a list

if isinstance(x, Iterator): x = list(x)

Well, if we have to add code, I guess supporting it is better, so I did that.

mrocklin · 2018-05-14T12:08:59Z

sparse/coo/core.py

+    if isinstance(x, Iterable):
+        return COO.from_iter(x, shape=shape)
+
+    raise NotImplementedError('Format not supported for conversion.')


It might be helpful to list here what was provided from the user and a set of valid values.

mrocklin · 2018-05-14T12:09:22Z

sparse/sparse_array.py

@@ -20,7 +20,7 @@ class SparseArray(object):

    def __init__(self, shape):
        if not isinstance(shape, Iterable):
-            shape = (int(shape),)
+            shape = (shape,)


Why this change?

If we provided a simple unsupported class that wasn't Iterable as a shape, this would break but give an unhelpful error message about not being able to cast to int. This way, the error message is more informative.

I tend to bunch in small bugfixes along with big PRs in order to get them in quickly, I don't really know how good of a habit this is.

It is common for numpy integers to sneak in here. Sometimes these cause issues later on. It would be good to ensure that all values of shape are eventually coerced to be normal Python integers.

I anticipated this already. This is already done, a few lines below this one. :-)

It was being done here an extra time before the check. I just changed that so it happens after the check, and gives a meaningful error message.

sparse/sparse/sparse_array.py

Lines 21 to 29 in b9fc91c

def __init__(self, shape):

if not isinstance(shape, Iterable):

shape = (int(shape),)

if not all(isinstance(l, Integral) and int(l) >= 0 for l in shape):

raise ValueError('shape must be an non-negative integer or a tuple '

'of non-negative integers.')

self.shape = tuple(int(l) for l in shape)

hameerabbasi · 2018-05-15T14:47:46Z

Any further changes required here?

mrocklin · 2018-05-15T14:52:59Z

No objection from me

hameerabbasi · 2018-05-15T15:22:21Z

This is in. Thanks for the review, @mrocklin; and thanks for the idea, @ahwillia!

Refactor and clean format conversions.

0a6b906

hameerabbasi requested a review from mrocklin May 9, 2018 11:14

hameerabbasi force-pushed the refactor-coo-creation branch from 581439c to c7a7ca3 Compare May 9, 2018 12:25

Add tests for missed branches.

ae38ae4

hameerabbasi force-pushed the refactor-coo-creation branch from c7a7ca3 to ae38ae4 Compare May 9, 2018 12:27

Add data broadcasting to COO constructor.

9aeda7a

Clarify docs.

014b050

ahwillia mentioned this pull request May 14, 2018

add full function to simplify COO creation #150

Closed

mrocklin reviewed May 14, 2018

View reviewed changes

hameerabbasi added 3 commits May 14, 2018 17:40

Explain supported formats.

14dc7b0

Support iterators as well as iterables.

3ab3ce2

Add test for Iterator.

5e0f36b

hameerabbasi merged commit 975a202 into pydata:master May 15, 2018

hameerabbasi deleted the refactor-coo-creation branch May 15, 2018 15:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor and clean format conversions. #152

Refactor and clean format conversions. #152

hameerabbasi commented May 9, 2018

hameerabbasi commented May 9, 2018

codecov-io commented May 9, 2018 •

edited

Loading

hameerabbasi commented May 12, 2018

mrocklin commented May 12, 2018 via email

hameerabbasi commented May 12, 2018

mrocklin commented May 12, 2018 via email

hameerabbasi commented May 12, 2018

ahwillia commented May 13, 2018

hameerabbasi commented May 13, 2018 •

edited

Loading

ahwillia commented May 14, 2018

mrocklin left a comment

mrocklin May 14, 2018

hameerabbasi May 14, 2018

mrocklin May 14, 2018

hameerabbasi May 14, 2018

mrocklin May 14, 2018

hameerabbasi May 14, 2018

mrocklin May 14, 2018

hameerabbasi May 14, 2018

mrocklin May 14, 2018

hameerabbasi May 14, 2018 •

edited

Loading

mrocklin May 14, 2018

hameerabbasi May 14, 2018

hameerabbasi commented May 15, 2018

mrocklin commented May 15, 2018

hameerabbasi commented May 15, 2018

	def __init__(self, shape):
	if not isinstance(shape, Iterable):
	shape = (int(shape),)

	if not all(isinstance(l, Integral) and int(l) >= 0 for l in shape):
	raise ValueError('shape must be an non-negative integer or a tuple '
	'of non-negative integers.')

	self.shape = tuple(int(l) for l in shape)

Refactor and clean format conversions. #152

Refactor and clean format conversions. #152

Conversation

hameerabbasi commented May 9, 2018

hameerabbasi commented May 9, 2018

codecov-io commented May 9, 2018 • edited Loading

Codecov Report

hameerabbasi commented May 12, 2018

mrocklin commented May 12, 2018 via email

hameerabbasi commented May 12, 2018

mrocklin commented May 12, 2018 via email

hameerabbasi commented May 12, 2018

ahwillia commented May 13, 2018

hameerabbasi commented May 13, 2018 • edited Loading

ahwillia commented May 14, 2018

mrocklin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hameerabbasi May 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hameerabbasi commented May 15, 2018

mrocklin commented May 15, 2018

hameerabbasi commented May 15, 2018

codecov-io commented May 9, 2018 •

edited

Loading

hameerabbasi commented May 13, 2018 •

edited

Loading

hameerabbasi May 14, 2018 •

edited

Loading