-
Notifications
You must be signed in to change notification settings - Fork 575
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
Allow Boolean mask indexing for param_shift_hessian
#2538
Conversation
Hello. You may have forgotten to update the changelog!
|
Codecov Report
@@ Coverage Diff @@
## master #2538 +/- ##
=======================================
Coverage 99.57% 99.57%
=======================================
Files 243 243
Lines 19444 19471 +27
=======================================
+ Hits 19361 19388 +27
Misses 83 83
Continue to review full report at Codecov.
|
@cvjjm Does this work the way you would expect? |
Thank you for adding this @dwierichs, I'll have a look at this shortly :) |
Scrolling through the code this looks very good! I won't have time until next week to try it in the specific case I am interested in but I am 99% sure this does the trick. Maybe it would be good to add one test case with params of "non-trivial shape". I.e., something like
In this case the Hessian is a 4 index tensor with shape
should be possible. If the current implementation does not support this, this can probably be added rather easily by raveling the params and mask and then, in the end, reshaping the hessian back to the proper shape. |
Great @cvjjm! Regarding higher-dimensional masks, note that currently the
Note that in general the difference between QNode arguments and tape arguments is not only a reshaping operation or permutation but any function. In particular, the classical Jacobian needs to be considered to implement this for general QNodes. On your end, however, it should be possible to construct intermediate tapes/QNodes that have flat parameters and thus a 2D mask anyways. @dime10 @josh146 what do you think about the priority of allowing |
@dwierichs This is definitely a point of confusion I've thought about before, in that |
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.
Thank you @dwierichs great work 💯
Co-authored-by: David Ittah <dime10@users.noreply.github.com>
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 @dwierichs, looks good 🎉
Implements the option to pass a Boolean
array_like
asargnum
toparam_shift_hessian
, to allow, for example, for computation of the diagonal alone. This is part of the feature request #2195 that was implemented to its major extent in #2319, but Boolean indexing was left open.Internally, any provided
argnum
is converted to such a Boolean array, so that the overall code complexity is not increased as far as I can tell.