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

Add nnz parameter to sparse.random #410

Merged
merged 4 commits into from
Sep 22, 2020
Merged

Add nnz parameter to sparse.random #410

merged 4 commits into from
Sep 22, 2020

Conversation

emilmelnikov
Copy link
Contributor

Currently, sparse.random uses the density argument to determine
the number of non-zero elements in the generated array. If the exact
number of non-zero elements is required, one might try to compute
density as nnz / np.prod(shape), which might give a wrong result
for large values due to a floating-point error.

This PR adds a new, optional nnz argument to sparse.random that,
if specified, overrides density and allows a caller to create
a random array with the exact number of non-zero elements.

Currently, `sparse.random` uses the `density` argument to determine
the number of non-zero elements in the generated array. If the exact
number of non-zero elements is required, one might try to compute
density as `nnz / np.prod(shape)`, which might give a wrong result
for large values due to a floating-point error.

This PR adds a new, optional `nnz` argument to `sparse.random` that,
if specified, overrides `density` and allows a caller to create
a random array with the exact number of non-zero elements.
@codecov
Copy link

codecov bot commented Sep 22, 2020

Codecov Report

Merging #410 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #410      +/-   ##
==========================================
+ Coverage   95.05%   95.06%   +0.01%     
==========================================
  Files          19       19              
  Lines        2810     2819       +9     
==========================================
+ Hits         2671     2680       +9     
  Misses        139      139              

Copy link
Collaborator

@hameerabbasi hameerabbasi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello. This is a nice addition, I'd just like to suggest tests for:

  • Err on something outside the unit interval.
  • Err when both density and nnz are provided.

Use pytest.raises (there are a couple of usages in this file).

sparse/_utils.py Outdated Show resolved Hide resolved
sparse/_utils.py Show resolved Hide resolved
@hameerabbasi
Copy link
Collaborator

I think there was a misunderstanding about what I meant: If exceptions are raised, they fail the test anyway (which is what we want). I meant you should test that:

  1. The exceptions you're raising are actually raised (use pytest.raises, grep for it in the codebase).
  2. Change the default density to None.
  3. Raise an error if both nnz and density are not None.
  4. If both are None, assume a density of 0.01.

Also simplify tests and remove unneeded nullcontext shim.

Co-authored-by: Hameer Abbasi <einstein.edison@gmail.com>
Copy link
Collaborator

@hameerabbasi hameerabbasi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor change more, otherwise LGTM.

@hameerabbasi hameerabbasi merged commit e9f2c87 into pydata:master Sep 22, 2020
@hameerabbasi
Copy link
Collaborator

Thanks, @emilmelnikov!

@emilmelnikov emilmelnikov deleted the random-nnz branch September 22, 2020 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants