-
Notifications
You must be signed in to change notification settings - Fork 60
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
DBF step optimization #1081
DBF step optimization #1081
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## dbf #1081 +/- ##
===========================================
- Coverage 100.00% 99.98% -0.02%
===========================================
Files 51 51
Lines 7583 7601 +18
===========================================
+ Hits 7583 7600 +17
- Misses 0 1 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
Thanks @MatteoRobbiati for implementing this.
I have a few suggestions down below.
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.
Thanks for all the updates @MatteoRobbiati.
Just a small comment regarding how to deal with hyperopt
again.
src/qibo/models/double_bracket.py
Outdated
|
||
import hyperopt |
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.
Now that we removed the hyperopt
dep from the main deps, when you try to import this module it will raise a ModuleNotFoundError
.
I think that you should move this import directly inside the hyperopt_step
possibly in a try-except block where you can tell the user to install hyperopt if they want to perform the optimiziation.
Perhaps at this point it is easier just to keep it as a dependency.
What do you recommend @scarrazza? Shall we add hyperopt
to the main deps or we keep it as an optional dep?
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.
If we like this setup, in which the user can set the space and the algorithm directly into the function, I think it is necessary to import hyperopt
at the beginning of the double_bracket.py
file.
Moreover, we never spent a lot of time in doing hyperoptimization of VQCs (number of layers, learning rate of the optimizers, etc).
I think adding an hyperoptimization tool as dependence of Qibo
can be useful in general.
If you prefere, we can choose between hyperopt
and Optuna
, I am quite indifferent on this.
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.
I think that you should move this import directly inside the hyperopt_step possibly in a try-except block where you can tell the user to install hyperopt if they want to perform the optimiziation.
I agree with this option
A first possible optimization of the step is implemented.
We use
hyperopt
to optimize the step considering a single flow execution.An example of usage:
And an example of benchmark between fixed step size (blue), and optimized step size:
Checklist: