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
[UnitaryHack] Implementation of Nelder Mead Optimizer #104
Conversation
@Gopal-Dahale Hi and thanks for this. There are some linting errors, could you fix them? |
Hi @dimkart, I have formatted the code and ran the two commands locally and both of them now give
|
Hi @dimkart, I see type check with |
Unfortunately there is no documentation for that, but running |
Thanks. I have fixed the |
@Gopal-Dahale Thanks, tests are now passing. This look already very good, and tomorrow we'll have a closer look at the code and we'll run the notebook to check performance. If you have already tried to run the notebook you might post here a screenshot of the trainer output for comparison. Keep an eye here for new comments :) |
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 your submission. I've had a skim through and it looks good so far, I just have a few comments about things that jumped out at me immediately. I would need more time to look through it in detail.
self.fsim[j] = self.func( | ||
diagrams, targets, self.sim[j] | ||
) | ||
except Exception: |
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.
Also here, it would be better to catch a more specific Exception
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 have updated it with a RuntimeError
.
- Using a RuntimeError exception in except block - Rename self.func to self.objective_func (descriptive) - Updated tests according to renaming
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've left a couple of comments, but in general it looks really great! Good work :)
self.fsim[k] = self.objective_func(diagrams, | ||
targets, | ||
self.sim[k]) | ||
except RuntimeError: |
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'm wondering why the objective function would triggers this error... Is there a reason for that or do we have a bug?
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.
This handles the case when the objective function does not return scalar values. I have updated the code for the objective function.
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 it's just used within this class, I think it would be clearer to define a custom exception and raise/catch since it is more specific
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.
Do you mean something like this?
class CustomError(RuntimeError):
pass
Any suggestion for the name?
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.
Maybe smething like LossEvalError
? @ianyfan?
- The objective fun is updated. It now checks whether the result is scalar or not and raises error if not which is handled by try except block - Added maxfev hyperparam for maximum function evaluations - Fixed typos and code style - Removed redundant code lines - Update state dict
Hi, @Thommy257 @ianyfan can you resolve the conversations? |
@Gopal-Dahale Hi, thanks for taking the time to respond to all comments -- the conversations will be resolved tomorrow by Ian and Thomas. |
Let's remove the notebook as well |
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.
Hi, sorry for the late review. Again, this all looks very good, but I've left some comments, mainly about code/docstring style, so you only need to address them if you get the chance to.
heuristic search approach, which means that it can sometimes | ||
converge to non-stationary points or suboptimal solutions. | ||
|
||
This implementation is based heavily scipy's optimize.minimize. See |
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.
based heavily on SciPy's `optimize.minimize` function.
model: QuantumModel, | ||
hyperparams: dict[str, float], | ||
loss_fn: Callable[[Any, Any], float], | ||
bounds: ArrayLike | None = None, |
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.
We don't need the extra comma at the end here
Hi @ianyfan, I had updated the code as per your comments. Can you resolve the conversations? |
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 this, good work! The issue will be assigned to you, however the PR will be merged a little later, after a new (already planned) new release.
Purpose
Implemented the Nelder Mead Optimizer.
Details
The optimizer along with tests. Also added the jupyter notebook used to test the optimizer. I have used 200 epochs instead of 100 as the optimizer required more epochs for convergence. This PR resolves #85.
Results
I ran two tests with
adaptive
equal toTrue
andFalse
for 200 epochs.adaptive=False
, final output is0.98555, 0.01445
adaptive=True
, final output is0.96625, 0.03375