-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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 V2 primitive container classes #11524
Add V2 primitive container classes #11524
Conversation
One or more of the the following people are requested to review 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 would approve, but since it is partially a self review, I think this PR needs to be approved by others 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.
Thanks @chriseclectic and @ikkoham .
I left some minor comments all to do with docstrings.
There are a bunch of # (C) Copyright IBM 2023.
that we might want to change to 2024. Otherwise, LGTM!
Also as @ikkoham said, FWIW, "partially a self review" applies to me too, as I wrote bits and pieces of a couple of things. |
Pull Request Test Coverage Report for Build 7465264931
💛 - Coveralls |
Add required container classes for V2 primitive implementations. Separated out from 11227 to unblock other PRs Co-Authored-By: Ikko Hamamura <[email protected]> Co-Authored-By: Takashi Imamichi <[email protected]> Co-Authored-By: Ian Hincks <[email protected]>
Co-authored-by: Ian Hincks <[email protected]>
c54b0d0
to
e9770db
Compare
* Add V2 primitive container classes Add required container classes for V2 primitive implementations. Separated out from 11227 to unblock other PRs Co-Authored-By: Ikko Hamamura <[email protected]> Co-Authored-By: Takashi Imamichi <[email protected]> Co-Authored-By: Ian Hincks <[email protected]> * Apply suggestions from code review Co-authored-by: Ian Hincks <[email protected]> * Update copyright to 2024 * Fix linting errors --------- Co-authored-by: Ikko Hamamura <[email protected]> Co-authored-by: Takashi Imamichi <[email protected]> Co-authored-by: Ian Hincks <[email protected]> Co-authored-by: Ian Hincks <[email protected]>
Add required container classes for V2 primitive implementations. Separated out from 11227 to unblock other PRs
Summary
This separates the common V2 primitives container classes from #11227 so discussion and modifications to SamplerV2 and EstimatorV2 can be worked on without blocking each other.
Note that this PR does not initially change any code from the subset of files from #11227 other than fixings some typos in doc strings.
Details and comments
My proposal to streamline getting the V2 primitives merged, and make it easier for the runtime to implement its primitives is to break #11227 and #11264 up 5 PRs (dependencies shown via nesting) for