-
Notifications
You must be signed in to change notification settings - Fork 585
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
[sc-69429] SPSA keyword argument bug fix #6027
Conversation
…lane into spsa-kwarg-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.
Is there a reason why no tests are added to address this fix? Was a test failing before? Or an issue that brought attention to this 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.
Thanks for the quick fix, @isaacdevlugt ! 🎉
I am wondering whether the function evaluation that needed fixing actually is required, or whether we can get the output shape from the evaluations we are doing anyways? And it might be nice to have a test that catches this bug, i.e. fails on current master.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #6027 +/- ##
==========================================
- Coverage 99.66% 99.65% -0.01%
==========================================
Files 427 427
Lines 41057 40764 -293
==========================================
- Hits 40919 40624 -295
- Misses 138 140 +2 ☔ 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.
Could continue discussing the necessity for the extra evaluation for extracting the shape, but as this is not the purpose of the PR, also good to merge without addressing it for now.
Thanks!
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.
Looks great! Thanks for working on this
Context:
SPSAOptimizer
'sstep_and_cost
method was ignoring keyword arguments in the objective function.Description of the Change:
SPSAOptimizer.step_and_cost
no longer ignores kwargs.Benefits: Your keyword arguments are heard <3
Possible Drawbacks: None
Related github issue: #6028