Implement rotosolve optimizers by ShuxiangCao · Pull Request #247 · tensorflow/quantum

ShuxiangCao · 2020-05-28T06:22:53Z

No description provided.

googlebot · 2020-05-28T06:22:57Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

ShuxiangCao · 2020-05-28T06:24:06Z

@googlebot I signed it!

ShuxiangCao · 2020-05-28T06:28:03Z

@googlebot I signed it!

googlebot · 2020-05-28T06:28:07Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

MichaelBroughton

Great first pass. Very well documented! Couple of comments and questions about your implementation:

You've implemented the __init__ logic of exposing rotosolve_minimize but there is no test for ensuring the api exposure is correct. Can you add a line in import_test.py to verify you are exposing this optimizer correctly ?
We have rotosolve_one_parameter_once that gets called by rotosolve_all_parameters_once. Do we need the functionality of rotosolve_one_parameter_once or can be it be removed and simplified ?

MichaelBroughton · 2020-05-29T09:52:42Z

+    minimize as rotosolve_minimize)
+
+# Utils for optimizers.
+from tensorflow_quantum.python.optimizers.utils import (function_factory)


Is function factory something you want a user to use ?

Yes, the user needs to wrap their model with this function_factory to expose the trainable parameters.

MichaelBroughton · 2020-05-29T09:53:07Z

+# limitations under the License.
+# ==============================================================================
+"""The rotosolve minimization algorithm
+"""


Nit: One line module description.

Oops. Fixed in a8ea8a7

MichaelBroughton · 2020-05-29T09:54:33Z

+        'converged',  # Scalar boolean tensor indicating whether the minimum
+        # was found within tolerance.
+        'num_iterations',  # The number of iterations of the BFGS update.
+        # The total number of objective evaluations performed.
+        'num_objective_evaluations',
+        'position',  # A tensor containing the last argument value found
+        # during the search. If the search converged, then
+        # this value is the argmin of the objective function.
+        # A tensor containing the value of the objective from previous iteration
+        'last_objective_value',
+        'objective_value',  # A tensor containing the value of the objective
+        # function at the `position`. If the search
+        # converged, then this is the (local) minimum of
+        # the objective function.
+        'tolerance',  # Define the stop criteria. Iteration will stop when the
+        # objective value difference between two iterations is smaller than
+        # tolerance
+        'solve_param_i',  # The parameter index where rotosolve is currently
+        # modifying. Reserved for internal use.


Formatting is a little hard to follow here. Could you make sure that the comments describing each element are always above the variable they are describing instead of sometimes being to the right and sometimes on top.

Yes, please use the same comment convention (including indentation) with that of TFP (here)

Fixed in 37e926f

Just realized the format checker doesn't happy with the indent like this. Should we change the configuration? @MichaelBroughton @jaeyoo

MichaelBroughton · 2020-05-29T10:00:51Z

+    The following example demonstrates the Rotosolve optimizer attempting
+     to find the minimum for two qubit ansatz expectation value.
+
+    ```python


This is a really good useage example. It is a little too complex for something that we want on the API docs. Do you think we could shorten things up a little bit ?

Also can you use >>> and ... like in https://github.com/tensorflow/quantum/blob/master/tensorflow_quantum/python/layers/high_level/pqc.py ?

Updated in 41b3161. Any changes suggested?

MichaelBroughton · 2020-05-29T10:01:22Z

+
+    Args:
+      expectation_value_function:  A Python callable that accepts
+        a point as a real `Tensor` and returns a tuple of `Tensor`s


Nit: tf.Tensor. Can you also outline the shape requirements for the input as well as the returned shape you get from the output ?

Fixed in f2b15d4

MichaelBroughton · 2020-05-29T10:02:29Z

+      tolerance: Scalar `Tensor` of real dtype. Specifies the gradient
+        tolerance for the procedure. If the supremum norm of the gradient
+        vector is below this number, the algorithm is stopped.
+      name: (Optional) Python str. The name prefixed to the ops created


Nit: Python str

Fixed in 25a75df

jaeyoo

Thank you for adding the first TFQ optimizer! I left some comments.

jaeyoo · 2020-05-29T15:08:19Z

+    """Return static shape of tensor `x` if available,
+     else `tf.shape(x)`.
+    Args:
+      x: `Tensor` (already converted).
+    Returns:
+      Numpy array (if static shape is obtainable), else `Tensor`.
+    """


we are using "four space" indent in comments, with a one-line description header with period.
e.g.

"""Static shape of a given tensor `x`. Return static shape of tensor `x` if available, else `tf.shape(x)`. Args: x: A `tf.Tensor` of which we want to know the shape. Returns: A numpy array (if static shape is obtainable), else `Tensor` containing a shape of `x`. """

Thanks! Fixed in 112400e

jaeyoo · 2020-05-29T15:08:33Z

+    """Return static value of tensor `x` if available, else `x`.
+    Args:
+      x: `Tensor` (already converted).
+    Returns:
+      Numpy array (if static value is obtainable), else `Tensor`.
+    """


Fixed in 112400e

jaeyoo · 2020-05-29T15:09:22Z

+    Args:
+      expectation_value_function:  A Python callable that accepts
+        a point as a real `Tensor` and returns a tuple of `Tensor`s


Fixed in 112400e

jaeyoo · 2020-05-29T15:09:46Z

+        point, or points when using batching dimensions, of the search
+        procedure. At these points the function value and the gradient
+         norm should be finite.
+      tolerance: Scalar `Tensor` of real dtype. Specifies the gradient


tf.Tensor

Fixed in 25a75df

jaeyoo · 2020-05-29T15:12:05Z

+        def _rotosolve_one_parameter_once(state):
+            """
+            Rotosolve a single parameter.
+            """


This internal function is hard to test. could you bring it out and write a test code?

one line description + arguments description please.

Well, this function is written here purely to help people read it, and it's a bit difficult to extract it out since too many variables are used in this closure. Also, the rest of the code is nothing but iterate over parameters and calling this function. Do you think it is okay to leave it as an internal function?

The docstring is fixed in 64f76c5

jaeyoo · 2020-05-29T15:16:17Z

+    """Tests for optimizer utils"""
+
+    def test_function_factory(self):
+        """Test the function_factory"""
+
+        class LinearModel(object):
+            """ A simple tensorflow linear model"""


period please, and please trim the meaningless white space in front of sentence.

Fixed in 564256f

jaeyoo · 2020-05-29T15:27:19Z

+
+    result = rotosolve_minimizer.minimize(
+        rotosolve_minimizer.function_factory(
+            model, hinge_loss, x_circ, Y), np.random.rand([2])


I am a little bit afraid of this kind of new and independent optimizer design. I understand that this design is conventional in TF Probability, but Tensorflow Keras provides its own Optimizer class to be inherited (e.g. see AdamOptimizer here).

To me, TFP's optimizer design seems like Keras' optimizer + custom trainer. I think it can't be runnable with keras model fit. I feel it's ok that it works well for saving weights and reloading to use the model after training, but I am not sure for the saving models with keras save function in the future. (it can save optimizer together)

What do you think of, @MichaelBroughton?

Making a new Keras Optimizers requires that optimizer to be gradient based. This new approach lets us depart from those standards. Rotosolve doesn't calculate gradients, just like nelder-mead in TFP doesn't use gradients.

You're right that it can't be runnable with keras model fit, but this seems to be the best workaround. I could ask the TFP people about this too. What do you think ?

Yes, I've checked gradient-free optimizers around several libraries, it is natural to implement gradient-free optimizers in an independent design. I thought the optimizer was designed enough to support both, but I was wrong. Then I agree with TFP's approach for the seamless ecosystem :) Thank you for your idea!

jaeyoo · 2020-05-29T15:27:45Z

+        is also of shape `[..., n]` like the input value to the function.
+        This must be a linear combination of quantum measurement
+        expectation value, otherwise this algorithm cannot work.
+      initial_position: Real `Tensor` of shape `[..., n]`. The starting


tf.Tensor

Fixed in 25a75df

jaeyoo · 2020-05-29T15:29:47Z

+        'converged',  # Scalar boolean tensor indicating whether the minimum
+        # was found within tolerance.
+        'num_iterations',  # The number of iterations of the BFGS update.
+        # The total number of objective evaluations performed.
+        'num_objective_evaluations',
+        'position',  # A tensor containing the last argument value found
+        # during the search. If the search converged, then
+        # this value is the argmin of the objective function.
+        # A tensor containing the value of the objective from previous iteration
+        'last_objective_value',
+        'objective_value',  # A tensor containing the value of the objective
+        # function at the `position`. If the search
+        # converged, then this is the (local) minimum of
+        # the objective function.
+        'tolerance',  # Define the stop criteria. Iteration will stop when the
+        # objective value difference between two iterations is smaller than
+        # tolerance
+        'solve_param_i',  # The parameter index where rotosolve is currently
+        # modifying. Reserved for internal use.


Yes, please use the same comment convention (including indentation) with that of TFP (here)

jaeyoo · 2020-05-29T15:36:53Z

+
+
+def _get_initial_state(initial_position, tolerance, expectation_value_function):
+    """Create RotosolveOptimizerResults with initial state of search ."""


please remove white space between the last character and period.

Fixed in 564256f

jaeyoo

Sorry for confusing, I request changes :)

ShuxiangCao · 2020-05-29T15:48:11Z

Thank you very much. I'll fix these issues soon.

…value function

ShuxiangCao · 2020-05-31T08:39:57Z

Great first pass. Very well documented! Couple of comments and questions about your implementation:

You've implemented the __init__ logic of exposing rotosolve_minimize but there is no test for ensuring the api exposure is correct. Can you add a line in import_test.py to verify you are exposing this optimizer correctly ?

We have rotosolve_one_parameter_once that gets called by rotosolve_all_parameters_once. Do we need the functionality of rotosolve_one_parameter_once or can be it be removed and simplified ?

Thanks for the comment again.

Done!
I abstracted rotosolve_one_parameter_once purely to make it easier to read. The rotosolve simply iterate over the parameters and call this function many times. I feel like it's better to leave it as an internal function, however, I am happy to embed it if people feel it is necessary.

ShuxiangCao · 2020-09-03T15:17:45Z

Thanks for the review, I have made most of the changes required. Can I have a bit more in detail how can I implement point 3 in best practice?

More sanity check tests to ensure that the number of function evalutions == max_iterations when the function terminates and didn't converge or if we set tolerance very high we do indeed terminate with function evaluations < max_iterations etc.

Also, I found the tests seems not working properly. I found some error when I execute the tests manually but then I try to run it with bazel it just simply passed. Can you help me have a look if I made some mistake which causes the tests got ignored?

MichaelBroughton · 2020-09-03T17:55:38Z

More sanity check tests to ensure that the number of function evalutions == max_iterations when the function terminates and didn't converge or if we set tolerance very high we do indeed terminate with function evaluations < max_iterations etc.

This would mean adding at least two more tests:

Running a test where when the rotosolve procedure terminates because evaluations == max_iterations. Then check that these two quantities are indeed equal.
Writing a test with a simple optimization problem with a large tolernace like 1e-2 and then ensure that rotosolve can terminate before reaching max_iterations.

Regarding the tests not working, can you post a small snippet or add in the changes with a comment to indicate which ones are not working ?

ShuxiangCao · 2020-09-07T14:28:41Z

Regarding the tests not working, can you post a small snippet or add in the changes with a comment to indicate which ones are not working ?

Please find all the changes in 25ecc76 . Before this commit the code should yield error since I called a function that does not even exist tf.sum and the syntax was wrong for scatter_update. Once I execute examples manually these problems got all exposed. However, when I was running it with bazel they were not spotted.

ShuxiangCao · 2020-09-07T14:37:02Z

Running a test where when the rotosolve procedure terminates because evaluations == max_iterations. Then check that these two quantities are indeed equal.

A new test called test_nonlinear_function_optimization did this check.

Writing a test with a simple optimization problem with a large tolernace like 1e-2 and then ensure that rotosolve can terminate before reaching max_iterations.

I have changed the ordinary test_function_optimization to do this check. In fact, it takes only 3 or 4 iterations to converge for the examples shown here.

MichaelBroughton · 2020-09-08T20:02:46Z

I'm guessing your tests aren't working because you don't have this at the bottom of your test module:

if __name__ == "__main__":
    tf.test.main()

Could you give that a try ?

ShuxiangCao · 2020-09-10T09:19:46Z

I'm guessing your tests aren't working because you don't have this at the bottom of your test module:
if __name__ == "__main__":
    tf.test.main()
Could you give that a try ?

Thanks for the help. I have fixed all the tests.

MichaelBroughton

Alrighty, this is ready to merge! Once we get these last round of minor nits changed, I can go ahead and merge this in :D

MichaelBroughton · 2020-09-12T22:42:43Z

+from __future__ import absolute_import
+from __future__ import division
+from __future__ import print_function


Are these needed ?

Removed in baf5e99

MichaelBroughton · 2020-09-12T22:46:09Z

+
+      arXiv:1905.09692
+
+    ### Usage:


Still have ### Usage: . We can just get rid of that altogether.

MichaelBroughton · 2020-09-12T22:46:49Z

+    The following example demonstrates the Rotosolve optimizer attempting
+     to find the minimum for two qubit ansatz expectation value.
+
+    Here is an example of optimize a function which consists summation of
+    a few sinusoids.


Do we want both of these descriptions or just one ?

Removed the first line in e9da877

MichaelBroughton · 2020-09-12T22:47:14Z

+    >>> import tensorflow_quantum as tfq
+    >>> import numpy as np
+    >>> import tensorflow as tf


Don't need these in the snippet, we assume people have done this already :)

Fixed in e9da877

MichaelBroughton · 2020-09-12T22:47:56Z

+    >>> min_value = -tf.math.reduce_sum(tf.abs(coefficient))
+    >>> func = lambda x:tf.math.reduce_sum(tf.sin(x) * coefficient)
+    >>> # Optimize the function with rotosolve, start with random parameters
+    >>> result = tfq.optimizers.rotosolve_minimizer.minimize( \


Don't need the \ can just do result = tfq.optimizers.rotosolve_minimizer.minimize(

Also shouldn't the function call be tfq.optimizers.rotosolve_minimize( and not tfq.optimizers.rotosolve_minimizer.minimize( ?

Fixed both in e9da877

MichaelBroughton · 2020-09-12T22:49:34Z

+    >>> print(result.converged)
+    tf.Tensor(True, shape=(), dtype=bool)
+    >>> print(result.objective_value)
+    tf.Tensor(-4.7045116, shape=(), dtype=float32)


In the doctests snippets we try to prefer just giving one output, and not using the print statement unless we really need to, would you mind changing these lines to just:

>>> result.objective_value tf.Tensor(-4.7045116, shape=(), dtype=float32)

Fixed in e9da877

MichaelBroughton · 2020-09-12T22:54:25Z

+from __future__ import absolute_import
+from __future__ import division
+from __future__ import print_function
+import math


math.pi is used here but np.pi is used in the tests. Could we change this module to also use np.pi just for consistency ?

Fixed in e9da877

ShuxiangCao · 2020-09-13T04:13:17Z

Alrighty, this is ready to merge! Once we get these last round of minor nits changed, I can go ahead and merge this in :D

Thanks for the review! I have fixed all these problems.

MichaelBroughton

Ready to merge :D

* Implement rotosolve optimizers * Fix import issue * Fix comments layout * Modify rotosolve example docstring * Remove meaningless whitespaces * Correct types in docstring * Change docstring indents * Trying to fix pytest faliure * Remove extra spances and change docstring layout * Fix typo in utils_test.py * Check the input / output shape requirement for rotosolve expectation value function * Add docstring for internal functions * Fix formats * Fix dependency * Fix import * Modify docstring layout * Fix format * Remove function factory (tensorflow#1) Remove function factory. * Fix format * Fix lint * Fix tests * Remove utils * Remove utils * Fix format and typos * Update examples * Fix lint * Fix typo * Switch to scatter_nd * Update rotosolve test * Reformat code * Update tests and comments * Update test * Fix lint * fix lint * Modify build docs, modify comments, change test to use built-in hinge loss * Fix format of build_docs.py * Change name of object value from previous iteration * Fix comments examples * Fix lint * Fix lint * Test tolerance criteria * Fix format * Fix tests * Fix format * Fix format * Change test * Fix tests * Fix typo in comment * Fix minor problems * Fix lint * Remove unused input * Update comment

Update version to 0.6.0

Implement rotosolve optimizers

b67c7f8

MichaelBroughton requested review from MichaelBroughton and jaeyoo May 29, 2020 09:50

MichaelBroughton requested changes May 29, 2020

View reviewed changes

jaeyoo reviewed May 29, 2020

View reviewed changes

jaeyoo requested changes May 29, 2020

View reviewed changes

ShuxiangCao added 7 commits May 31, 2020 06:11

Fix import issue

a8ea8a7

Fix comments layout

37e926f

Modify rotosolve example docstring

41b3161

Remove meaningless whitespaces

564256f

Correct types in docstring

25a75df

Change docstring indents

112400e

Trying to fix pytest faliure

26403bd

This comment has been minimized.

Sign in to view

ShuxiangCao added 4 commits May 31, 2020 08:14

Remove extra spances and change docstring layout

7f8bfb4

Fix typo in utils_test.py

490331f

Check the input / output shape requirement for rotosolve expectation …

f2b15d4

…value function

Add docstring for internal functions

64f76c5

ShuxiangCao requested review from MichaelBroughton and jaeyoo May 31, 2020 08:41

ShuxiangCao added 4 commits May 31, 2020 08:46

Fix formats

f8ec7fe

Fix dependency

5f5508c

Fix import

62105dc

Modify docstring layout

aa74043

ShuxiangCao requested a review from MichaelBroughton September 3, 2020 15:18

ShuxiangCao added 2 commits September 7, 2020 22:28

Merge branch 'master' into master

9c4fe84

Test tolerance criteria

5535067

Fix format

3ba151c

ShuxiangCao and others added 7 commits September 10, 2020 16:43

Merge branch 'master' into master

72b43bc

Fix tests

3bc4bf2

Fix format

5004e92

Fix format

52984ca

Merge branch 'master' of github.com:SachinCompton/quantum

8e1d701

Change test

f4ddd7d

Fix tests

25c1f65

Fix typo in comment

2a425e7

MichaelBroughton requested changes Sep 12, 2020

View reviewed changes

ShuxiangCao added 5 commits September 13, 2020 11:57

Merge branch 'master' into master

1a40bf9

Fix minor problems

e9da877

Fix lint

fef587c

Remove unused input

baf5e99

Update comment

5217868

ShuxiangCao requested a review from MichaelBroughton September 13, 2020 04:17

MichaelBroughton approved these changes Sep 14, 2020

View reviewed changes

MichaelBroughton merged commit 5e5ed34 into tensorflow:master Sep 14, 2020

jaeyoo pushed a commit to jaeyoo/quantum that referenced this pull request Mar 30, 2023

Merge pull request tensorflow#247 from quantumlib/release-v0_6_0

23b0d3d

Update version to 0.6.0



		def _get_initial_state(initial_position, tolerance, expectation_value_function):
		"""Create RotosolveOptimizerResults with initial state of search ."""

Conversation

ShuxiangCao commented May 28, 2020

Uh oh!

googlebot commented May 28, 2020

What to do if you already signed the CLA

Individual signers

Corporate signers

Uh oh!

ShuxiangCao commented May 28, 2020

Uh oh!

ShuxiangCao commented May 28, 2020

Uh oh!

googlebot commented May 28, 2020

Uh oh!

MichaelBroughton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ShuxiangCao May 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ShuxiangCao May 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaeyoo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ShuxiangCao May 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ShuxiangCao May 31, 2020 •

edited

Loading

ShuxiangCao May 31, 2020 •

edited

Loading

ShuxiangCao May 31, 2020 •

edited

Loading

jaeyoo May 30, 2020 •

edited

Loading

ShuxiangCao commented May 31, 2020 •

edited

Loading

ShuxiangCao commented Sep 7, 2020 •

edited

Loading