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

Dev #186

Merged
merged 15 commits into from
Sep 25, 2023
Merged

Dev #186

merged 15 commits into from
Sep 25, 2023

Conversation

FelixXu35
Copy link
Contributor

I partially rewrote the portfolio optimisation tutorial into two new ones.

Mixers and cvar are discussed in these two tutorials, respectively.

They passed the grammar, format and sphinx check.

If the new tutorials work, I am going to delete the old one.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

docs/source/tutorials/QUBO_problem.ipynb Outdated Show resolved Hide resolved
docs/source/tutorials/QUBO_problem.ipynb Outdated Show resolved Hide resolved
docs/source/tutorials/QUBO_problem.ipynb Outdated Show resolved Hide resolved
@@ -0,0 +1,580 @@
{
Copy link
Contributor

@refraction-ray refraction-ray Aug 24, 2023

Choose a reason for hiding this comment

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

it is not directly comparable in this figure, since the loss definition (different alpha) are different?


Reply via ReviewNB

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes that's right. How low the loss can reach is not comparable. But you may find that with smaller alpha, the QAOA converges faster.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hang on. I suddenly realise that they are comparable. In the ideal scenario, where the best result can be measured with 100% probability, their loss/cost should be the same.

Copy link
Contributor

Choose a reason for hiding this comment

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

but except the perfect case, they are not comparable

@@ -0,0 +1,481 @@
{
Copy link
Contributor

@refraction-ray refraction-ray Aug 24, 2023

Choose a reason for hiding this comment

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

comparison with the one without mixer (plain QAOA)?


Reply via ReviewNB

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is compared with the one using standard X mixer, which is shown in the convergence figure and probability table above. With XY mixer, the convergence procedure isn't improved. But the overlap (probability of finding the best/correct result) increases.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe include both lines (X and XY) in the same plot so the reader can see the comparison?

@refraction-ray
Copy link
Contributor

also please modify this file accordingly is the same PR: https://github.com/tencent-quantum-lab/tensorcircuit/blob/master/docs/source/tutorial.rst

@codecov
Copy link

codecov bot commented Aug 24, 2023

Codecov Report

Merging #186 (31d47ad) into master (b96a1b1) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #186   +/-   ##
=======================================
  Coverage   75.50%   75.50%           
=======================================
  Files          67       67           
  Lines       10668    10668           
=======================================
  Hits         8055     8055           
  Misses       2613     2613           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@refraction-ray
Copy link
Contributor

please use all lower letters for the file name, qubo instead of QUBO

@@ -0,0 +1,485 @@
{
Copy link
Contributor

@JAllcock JAllcock Aug 29, 2023

Choose a reason for hiding this comment

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

typo: 'brutal force' -> 'brute force'

Might be good to remind the reader about what the cost function is, e.g. "We first use brute force search to calculate the cost $x^TQx$ for each $x\in\{0,1\}^n$, where $n$ is the number of stocks.


Reply via ReviewNB

@@ -0,0 +1,485 @@
{
Copy link
Contributor

@JAllcock JAllcock Aug 29, 2023

Choose a reason for hiding this comment

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

'trained for 1000 times' -> 'trained for 1000 iterations'

Small thing, but earlier we talk about the 'cost' function, now we call it a 'loss'; maybe use the same term for both?


Reply via ReviewNB

@@ -0,0 +1,485 @@
{
Copy link
Contributor

@JAllcock JAllcock Aug 29, 2023

Choose a reason for hiding this comment

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

typo: 'thereby ensuring consistency with the classical approach' -> 'which is consistent with what we found with classical brute force search.'


Reply via ReviewNB

@@ -0,0 +1,485 @@
{
Copy link
Contributor

@JAllcock JAllcock Aug 29, 2023

Choose a reason for hiding this comment

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

There seems to be a lot of overlap between this tutorial and the other one 'qubo_problem.ipynb'. Better to split it roughly as (i) basic portfolio optimisation with QAOA (ii) advanced QAOA for portfolio optimisation?

In the first one you just do QAOA with X mixer; in the second you add XY mixer and also CVaR?


Reply via ReviewNB

@@ -0,0 +1,580 @@
{
Copy link
Contributor

@JAllcock JAllcock Aug 29, 2023

Choose a reason for hiding this comment

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

seems to be problem with latex rendering the Ising Hamiltonian


Reply via ReviewNB

@@ -0,0 +1,580 @@
{
Copy link
Contributor

@JAllcock JAllcock Aug 29, 2023

Choose a reason for hiding this comment

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

typo: 'is generally considered NP hard' -> 'is NP hard'

typo: 'we wish to minimise the cost function int he form of' -> 'we wish to minimise a cost function of the form'

The term 'ansatz' is mentioned a few times but isn't really defined

typo: 'this cost fucntion' -> 'this cost function'

typo: 'Below, the code of solving QUBO problem is shown' -> 'Below, the code used to solve this QUBO problem is shown.'


Reply via ReviewNB

@@ -0,0 +1,580 @@
{
Copy link
Contributor

@JAllcock JAllcock Aug 29, 2023

Choose a reason for hiding this comment

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

typo: 'the we see below' -> we see below

typo: 'combination' -> 'combination' (although maybe better to say 'give the best solution'


Reply via ReviewNB

@@ -0,0 +1,580 @@
{
Copy link
Contributor

@JAllcock JAllcock Aug 29, 2023

Choose a reason for hiding this comment

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

maybe add link to CVaR paper?


Reply via ReviewNB

@@ -0,0 +1,580 @@
{
Copy link
Contributor

@JAllcock JAllcock Aug 29, 2023

Choose a reason for hiding this comment

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

typo: 'let's define a function brutally calculate the costs (classical methods)' -> 'let's define a function to classically brute-force calculate all possible combinations of stocks and their associated cost functions.'


Reply via ReviewNB

@@ -0,0 +1,580 @@
{
Copy link
Contributor

@JAllcock JAllcock Aug 29, 2023

Choose a reason for hiding this comment

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

indicate what the legend (1, 0.25, 0.1) mean in both graphs?


Reply via ReviewNB

@@ -318,9 +330,10 @@ def QUBO_QAOA_cvar(
:param maxiter: The maximum number of iterations for the optimization. Default is 1000.
:return: The optimized parameters for the ansatz circuit.
"""
tf.config.run_functions_eagerly(True)
Copy link
Contributor

Choose a reason for hiding this comment

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

I dont think this is necessary for tf 2.0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If this sentence is deleted, there is going to be an error like:

    OperatorNotAllowedInGraphError: Iterating over a symbolic `tf.Tensor` is not allowed: AutoGraph did convert this function. This might indicate you are trying to use an unsupported feature.

Copy link
Contributor

Choose a reason for hiding this comment

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

exactly, this is because you impl is not jittable as I suggested below. By enabling this, the jit is closed. so no more no less, jit is still not enabled

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. Will fix it very soon.


# Iterate over the sorted results and calculate CVaR.
while sump < percent:
if round(sump + p[count], 6) >= percent:
if tf.math.round(sump + p[count], 6) >= percent:
Copy link
Contributor

Choose a reason for hiding this comment

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

concerned on whether this is really jittable, due to the if tensor pattern here, where tensor directly depends on the jittable input argument of the function

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the correct way is to build a sign function depending on the threshold to filter the results with higher loss without using the if tensor pattern

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your advice. Let me give it a try.

@FelixXu35
Copy link
Contributor Author

I refactored cvar_value, cvar_from_circuit, and cvar_from_expectation.
Now jit=True saves 1/3 time.

@refraction-ray
Copy link
Contributor

refraction-ray commented Sep 15, 2023

Now jit=True saves 1/3 time

This is subtle, I expect orders of magnitude acceleration with jit. The implementation looks correct to me.

Copy link
Contributor

@JAllcock JAllcock left a comment

Choose a reason for hiding this comment

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

Number of typos, and places where the explanation e.g. docstring can be made clearer.

# moved
def QUBO_to_Ising(Q: List[list]) -> Tuple[List[list], list, float]:
"""
Cnvert the Q matrix into a the indication of pauli terms, the corresponding weights, and the offset.
Copy link
Contributor

Choose a reason for hiding this comment

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

spelling: Cnvert... into a the indication...
spelling: pauli -> Pauli

Copy link
Contributor

Choose a reason for hiding this comment

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

docstring would benefit from a simple example


def Ising_loss(c: tc.Circuit, pauli_terms: List[list], weights: list) -> float:
"""
computes the loss function for the Ising model based on a given quantum circuit,
Copy link
Contributor

Choose a reason for hiding this comment

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

spelling: computes -> Computes

docstring would benefit from an explanation of what the loss function is

:param ansatz: The ansatz function to be used for the QAOA.
:param nlayers: The number of layers in the QAOA ansatz.
:param iterations: The number of iterations to run the optimization.
:param vvag (optional): A flag indicating whether to use vectorized variational adjoint gradient. Default is False.
Copy link
Contributor

Choose a reason for hiding this comment

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

vvag is 'vectorized value and gradient'

Performs the QAOA on a given QUBO problem.

:param Q: The n-by-n square and symmetric Q-matrix representing the QUBO problem.
:param ansatz: The ansatz function to be used for the QAOA.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you should say :param ansatz should be supplied e.g. by using the QAOA_ansatz_for_Ising function imported from templates.blocks?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is 'ansatz' the circuit or is it the cost function defined by the circuit? below you define loss_val_grad in terms of the ansatz.

return params


# calcelled
Copy link
Contributor

Choose a reason for hiding this comment

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

typo

) -> float:
"""
Directly calculate the Conditional Value at Risk (CVaR) from a circuit.
The CVaR depends on a bunch of measurements.
Copy link
Contributor

Choose a reason for hiding this comment

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

'depends on a bunch of measurements' sounds too informal. Can we express this more formally?

return cvar_result


def cvar_from_expectation(circuit, Q, alpha: float) -> float:
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like the main difference between this and cvar_from_circuit is that cvar_from_circuit uses empirical measurement results, while cvar_from_expectation classically computes the probabilities of all outcomes. I think we should mention this.

:param nlayers: The number of layers in the QAOA ansatz.
:pauli_terms: A list of Pauli terms, where each term is represented as a list of 0/1 series.
:param weights: A list of weights corresponding to each Pauli term.
:param mixer: mixer type. The options are "X", "XY_ring", "XY_par_ring", "XY_full", and "QAMPA".
Copy link
Contributor

Choose a reason for hiding this comment

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

I think an explicit example would help in the docstring

Tensor = Any


# moved
Copy link
Contributor

Choose a reason for hiding this comment

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

you moved it to conversions right? why is it still here?


def QUBO_to_Ising(Q: List[list]) -> Tuple[List[list], list, float]:
"""
Cnvert the Q matrix into a the indication of pauli terms, the corresponding weights, and the offset.
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: Cnvert
typo: into a the indication of
typo: Pauli should be capitalized

Copy link
Contributor

@refraction-ray refraction-ray left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the nice contribution!

@refraction-ray refraction-ray merged commit 31d47ad into tencent-quantum-lab:master Sep 25, 2023
4 checks passed
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.

3 participants