initializing AQGD optimizer

[kareem1925]

Informations

• Qiskit Aqua version:
  'qiskit-aqua': '0.6.1'
• Python version:
  3.6.9
• Operating system:
  ubuntu 18.04

What is the current behavior?

an error happens when i try to initialize the AQGD

SchemaError: True is not of type 'number'

Failed validating 'type' in metaschema['properties']['properties']['additionalProperties']['properties']['exclusiveMaximum']:
{'type': 'number'}

On schema['properties']['momentum']['exclusiveMaximum']:
True

Steps to reproduce the problem

from qiskit.aqua.components import optimizers
opt = optimizers.AQGD()

What is the expected behavior?

I expect that nothing wrong happens just like the following code:
opt = optimizers.ADAM(maxiter=20,lr=0.2)
there is no errors while executing this line

Suggested solutions

I believe that there is something wrong happens while validating the configuration file of this optimizer. I think modifying it may help, I'm still searching for it

[woodsp-ibm]

At some point there was a change to the json schema specification definition for exclusiveMaximum and we corrected our code to match the latest schema spec in 0.6.2. Try installing the latest released version of Aqua which is 0.6.2 pip install -U qiskit-aqua. This was fixed for that version.

[kareem1925]

thanks a lot. I had to upgrade the whole qiskit package to make it work perfectly (Y)
sudo -H pip install qiskit --upgrade
thanks again (Y)

[kareem1925]

I'm sorry to close the issue without permission.
actually there is something else:
in this line @woodsp-ibm
i believe you should omit the "not" because the while loop produces an error. when I changed it everything works perfectly.
another thing:
the typical value of the momentum in neural networks is 0.9. but you set it to 0.25 and then subtract that from 1. it confused me and may confuse other people I hope you consider changing it to the default value like other deep leanring libraries.
I'll be glad if you allow me to help with that.

[woodsp-ibm]

I had expected you to close this issue since you created it and a fix was given which worked - you are permitted to do this as well as re-open, so no problem :)

The issue arises now from this line
https://github.com/Qiskit/qiskit-aqua/blob/00ed44162b9cae91ef38098c0ed8bb1ee0c8e9fc/qiskit/aqua/components/optimizers/aqgd.py#L67
that was not in the original code but got added because of lint check failure. The converged method check
https://github.com/Qiskit/qiskit-aqua/blob/00ed44162b9cae91ef38098c0ed8bb1ee0c8e9fc/qiskit/aqua/components/optimizers/aqgd.py#L132
needed that to create the instance variable if it did not exist - which first time in that call in the past it would have been correct. Since the variable has been created in the current code - which maybe is better option - the test now should more be if self._previous_loss is None: rather than using the has_attr logic.

As to momentum its used as per this line where both the value and 1 minus the value are used. Is it this line you are questioning?
https://github.com/Qiskit/qiskit-aqua/blob/00ed44162b9cae91ef38098c0ed8bb1ee0c8e9fc/qiskit/aqua/components/optimizers/aqgd.py#L114

[kareem1925]

hi, thanks for your reply.

needed that to create the instance variable if it did not exist - which first time in that call in the past it would have been correct. Since the variable has been created in the current code - which maybe is better option - the test now should more be if self._previous_ls_loss is None: rather than using the has_attr logic.

should I change it and make a pull request? it's working now on my machine and also tested it on a windows machine and colab

As to momentum its used as per this line where both the value and 1 minus the value are used. Is it this line you are questioning?
https://github.com/Qiskit/qiskit-aqua/blob/00ed44162b9cae91ef38098c0ed8bb1ee0c8e9fc/qiskit/aqua/components/optimizers/aqgd.py#L114

yeah that's the line. I'm actually accustomed to 0.9 as it's the default value in tensorflow and pytorch libraries. when i was trying to explain quantum gradients to my colleagues it stopped me for a while actually 😅 so i had to implement the whole algorithm from scratch to keep it consistent with other ML libraries flow. it's only a suggestion.

[woodsp-ibm]

Hi, sure having a PR to fix the if self_previous_loss test would be great.

I recall the defaults were chosen based on some testing that was done - mostly to do analytic gradient of RY var form when used in VQE doing a ground state energy computation, where these seemed to do well. I was looking at pytorch SQD and they default to 0 - were you looking at some other code?
https://github.com/pytorch/pytorch/blob/927c2a02b0b29a0fafcced8d65896dd417023067/torch/optim/sgd.py#L51
It is a default after all and you are of course free to use 0.9 rather that 0.25

[kareem1925]

of course, the default would be zero for a pure SGD as you mentioned sir, but with momentum most of the people use 0.9 like this simple example from keras. the first code. i was not aware of the RY thing. I'll check it some other time.
and I'll work on the PR right away (Y) thanks for your permission

[woodsp-ibm]

Closing as this was fixed by #770

[kareem1925]

hi @woodsp-ibm, i'm now having the same issue because of the while loop 😢
in this line. #770 I thought that this pull request should have solved the problem
and it actually solved the problem locally on my device

[woodsp-ibm]

Hi @kareem1925 the PR was for the master branch. This has not yet been released but will be part of the upcoming 0.7.0 currently planned for March. In the master the line has changed https://github.com/Qiskit/qiskit-aqua/blob/769ca8f7fbb91fcfb4dd47c956b6358bb53212ef/qiskit/aqua/components/optimizers/aqgd.py#L141 so installing qiskit from source (clones) of the repo should work. So if you installed a stable release 0.6.4 or earlier it will still be as you reported and only master was changed by the PR.