-
Notifications
You must be signed in to change notification settings - Fork 39
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
Add mcmc sampler #384
Add mcmc sampler #384
Conversation
Hello. You may have forgotten to update the changelog!
|
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.
Just a quick one for the failures. Will do a more in-depth review later.
Co-authored-by: Lee James O'Riordan <mlxd@users.noreply.github.com>
Codecov Report
@@ Coverage Diff @@
## master #384 +/- ##
========================================
Coverage 99.82% 99.82%
========================================
Files 49 50 +1
Lines 4517 4624 +107
========================================
+ Hits 4509 4616 +107
Misses 8 8
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
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 @trevor-vincent
Some suggestions regarding the test coverage. Also, can we add a docstring to each class and associated method?
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 @multiphaseCFD @trevor-vincent!
This looks great! How will a PL user enable the MCMC sampling mode? Aside from directly calling the device methods (which may not be known to non-expert users), it would be nice for users to be able to opt-in to MCMC sampling easily. For example, when instantiating the device qml.device("lightning.qubit", shots=100, mcmc=True)
- though I'm curious if there has been any prior discussion on this?
Hi @trbromley , That's a good point! We didn't discuss that before. Let me discuss with @mlxd , then get back to you. |
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.
Nothing more from me --- thanks for the work everybody!
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.
Just made a couple suggestions. Happy to approve!
Co-authored-by: Vincent Michaud-Rioux <vincentm@nanoacademic.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 @multiphaseCFD! I've left a few suggestions on the documentation side, but otherwise looks great to me!
Co-authored-by: Tom Bromley <49409390+trbromley@users.noreply.github.com>
Co-authored-by: Tom Bromley <49409390+trbromley@users.noreply.github.com>
Co-authored-by: Tom Bromley <49409390+trbromley@users.noreply.github.com>
Co-authored-by: Tom Bromley <49409390+trbromley@users.noreply.github.com>
Co-authored-by: Tom Bromley <49409390+trbromley@users.noreply.github.com>
Co-authored-by: Tom Bromley <49409390+trbromley@users.noreply.github.com>
No description provided.