add full function to simplify COO creation

[ahwillia]

This package may end up being very important to me, so I wanted to dip my toes in and try adding a simple extension in case I want to add something more substantial later.

The basic idea is to make array creation easier when all the data values are the same (I think this is incredibly common, e.g. if you are representing sparse graph structure)

sparse.full(np.array([[0,1,2],[0,1,2]]), 1).todense()

array([[1, 0, 0],
       [0, 1, 0],
       [0, 0, 1]])

A few things I wasn't sure on...

• how to document **kwargs
• Should I be accepting non-array objects for coords? In particular should I add something to the effect of coords = np.asarray(coords) at the top of my function

Hope that this is welcome and within scope. Thanks!

[codecov-io]

Codecov Report

Merging #150 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #150      +/-   ##
==========================================
+ Coverage   95.77%   95.78%   +0.01%     
==========================================
  Files          10       10              
  Lines        1183     1187       +4     
==========================================
+ Hits         1133     1137       +4     
  Misses         50       50

Impacted Files	Coverage Δ
sparse/coo/__init__.py	100% <ø> (ø)	⬆️
sparse/coo/core.py	93.71% <100%> (ø)	⬆️
sparse/coo/common.py	96.19% <100%> (+0.07%)	⬆️

---

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...2070086. Read the comment docs.

[hameerabbasi]

Hello, @ahwillia! A big thanks for adding to the project. Every contribution is welcome. As for your questions:

• You can accept non-array values for coords, the COO constructor fixes those for you. See

  sparse/sparse/coo/core.py

  Line 227 in 3a53e87

  self.coords = np.asarray(coords)

• I would recommend mirroring the signature of np.full rather than adding **kwargs, and then documenting those keywords as usual. If you prefer to keep **kwargs, though, just document it like this:

kwargs : dict, optional
    Additional arguments to pass to ``np.full``.

• Your PR is fairly complete, but it's missing one thing. In this file, you need to add full in its proper place alphabetically: https://github.com/pydata/sparse/blob/a95b372df35a4f771a5b045c487079e64477d38c/docs/generated/sparse.rst

[hameerabbasi]

compare_dtype is on by default, no need to add it here.

[hameerabbasi]

See comment to above.

[mrocklin]

I wonder if it would make sense to have the COO constructor do this automatically

x = COO(coords=..., data=1)

To my knowledge this doesn't conflict with any other potential input.

[hameerabbasi]

Yes, we can. The question is whether we should add this to the constuctor, which is already quite bloated (both in terms of using and coding).

[mrocklin]

I agree that it's somewhat bloated, however to me this doesn't seem to increase that bloat substantially.

…

On Sat, May 5, 2018 at 10:00 AM, Hameer Abbasi ***@***.***> wrote: Yes, we can. The question is whether we should add this to the constuctor, which is already quite bloated (both in terms of using and coding). — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#150 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AASszBBFqtvk8Bw-uI4JwZjCv9EsxOmnks5tvbBngaJpZM4TzdB0> .

[hameerabbasi]

Fair enough. We can just replace self.data = ... with self.data = np.broadcast_to(..., coords.shape[1]).

[ahwillia]

Thanks for the feedback.

I would recommend mirroring the signature of np.full rather than adding **kwargs, and then documenting those keywords as usual.

I am actually passing kwargs to the COO constructor not to np.full. My reasoning here is that the constructor signature may change? This might be an argument for instead adding this functionality to the constructor as @mrocklin suggested.

As someone who just started playing around with the package I actually find the COO constructor a bit confusing. I would be in favor of keeping sparse.full(...) because it matches a common function in numpy and I think familiar users will know how to use it immediately.

I don't mind also adding this to the COO constructor as well -- having multiple ways to do something isn't necessarily a bad thing. On the other hand, I kind of wish that the COO constructor didn't accept a dense numpy array and instead threw an error pointing to COO.from_numpy(x). I managed to confuse myself quite a bit while initially playing around with this package.

Would it make sense to hold off on this PR and open an issue/discussion on the COO constructor behavior?

[hameerabbasi]

Sure. I'm +0.5 on changing constructor behaviour. @ahwillia would you mind opening that issue?

[ahwillia]

Closing this in favor of #152