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

NumPy random generators #1267

Merged
merged 14 commits into from
May 5, 2021
Merged

NumPy random generators #1267

merged 14 commits into from
May 5, 2021

Conversation

albi3ro
Copy link
Contributor

@albi3ro albi3ro commented May 2, 2021

Currently, the recommended way of creating random numbers with NumPy involves creating a local generator object, and then querying that object for random numbers

import numpy as np
rng = np.default_rng()
rng.random((3,2))

Currently, we cannot use this method with PennyLane wrapped NumPy. This PR adds a wrapped generator object and default_rng generator constructor to PennyLane's NumPy random. It also import the bitstring generators into the random namespace. Now we can call

from pennylane import numpy as np
rng = np.default_rng()
rng.random((3,2))

and get an array with a requires_grad attribute,

@josh146 josh146 linked an issue May 3, 2021 that may be closed by this pull request
@josh146
Copy link
Member

josh146 commented May 3, 2021

@albi3ro, I'll attach the WIP label until tests are added 🙂

@josh146 josh146 added the WIP 🚧 Work-in-progress label May 3, 2021
@codecov
Copy link

codecov bot commented May 3, 2021

Codecov Report

Merging #1267 (362251f) into master (cdef772) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1267   +/-   ##
=======================================
  Coverage   98.14%   98.14%           
=======================================
  Files         147      147           
  Lines       11223    11242   +19     
=======================================
+ Hits        11015    11034   +19     
  Misses        208      208           
Impacted Files Coverage Δ
pennylane/numpy/random.py 100.00% <100.00%> (ø)

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 cdef772...362251f. Read the comment docs.

@albi3ro albi3ro added review-ready 👌 PRs which are ready for review by someone from the core team. and removed WIP 🚧 Work-in-progress labels May 3, 2021
@albi3ro albi3ro removed the review-ready 👌 PRs which are ready for review by someone from the core team. label May 3, 2021
@albi3ro albi3ro requested a review from josh146 May 3, 2021 17:47
Copy link
Member

@josh146 josh146 left a comment

Choose a reason for hiding this comment

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

Nice work @albi3ro! This is a great add. Impressive testing as well!

My only big question mark is the packaging package, to make sure we don't cause import errors for end-users

.github/CHANGELOG.md Outdated Show resolved Hide resolved
@@ -15,7 +15,44 @@
This package provides a wrapped version of autograd.numpy.random, such that
it works with the PennyLane :class:`~.tensor` class.
"""
from packaging.version import parse as parse_version
Copy link
Member

Choose a reason for hiding this comment

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

Is the packaging library a part of the Python standard library? I was a bit confused, since it's available on PyPI, but at the same time, you didn't need to add it to the requirements.txt 🤔

This is nice to know though, in other parts of the library we depend on the semantic_version==2.6 package for this!

from autograd.numpy import random as _random
from .wrapper import wrap_arrays
from numpy import __version__ as np_version
from numpy.random import MT19937, PCG64, Philox, SFC64 # pylint: disable=unused-import
Copy link
Member

Choose a reason for hiding this comment

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

How come this is imported if not used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We want these in the namespace, so someone can call qml.numpy.random.MT19937, instead of having to call qml.numpy.random._random.MT19937


for name in dir(_random.Generator):
if name[0] != "_":
self.__dict__[name] = tensor_wrapper(getattr(super(), name))
Copy link
Member

Choose a reason for hiding this comment

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

Nice! I tested this out, it works really well.

Copy link
Member

Choose a reason for hiding this comment

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

I had a look through all methods - it looks like most of them do return ndarray objects, so it should be good 👍 Are we inadvertently wrapping any that don't return arrays?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The methods will return a single scalar if not called with a size. If a size is provided, then it returns an ndarray. I've thought about explicitly writing out the names of all the generators that need to be wrapped, but decided against it.

mat1_2 = rng1.normal(size=size)
mat2_2 = rng2.normal(size=size)

assert np.all(mat1_2 == mat2_2)
Copy link
Member

Choose a reason for hiding this comment

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

Great tests!

@albi3ro albi3ro merged commit 6e8efa8 into master May 5, 2021
@albi3ro albi3ro deleted the np_random_generators branch May 5, 2021 16:03
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.

Update NumPy random module wrapping
2 participants