-
-
Notifications
You must be signed in to change notification settings - Fork 131
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
Conversation
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 Report
@@ 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 |
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.
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
andnnz
are provided.
Use pytest.raises
(there are a couple of usages in this file).
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:
|
Also simplify tests and remove unneeded nullcontext shim. Co-authored-by: Hameer Abbasi <einstein.edison@gmail.com>
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.
One minor change more, otherwise LGTM.
Thanks, @emilmelnikov! |
Currently,
sparse.random
uses thedensity
argument to determinethe 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 resultfor large values due to a floating-point error.
This PR adds a new, optional
nnz
argument tosparse.random
that,if specified, overrides
density
and allows a caller to createa random array with the exact number of non-zero elements.